Discuss Kourse 1

Sat Dec 27, 2008 3:55 pm

Discuss Kourse 1

Sat Dec 27, 2008 3:55 pm

Here you can discuss Kourse 1 apart from the main action.

forum.kde.org
Communicate.

Thu Jan 01, 2009 7:11 am

RE: Discuss Kourse 1

Thu Jan 01, 2009 7:11 am

I think bugfixing ksnapshot was a good choice for kourse 1. It is a relatively simple and small program to understand, so people new to KDE development can follow the logic of the codes quite ok.

Thanks to msoeken, the mentor for kourse 1, for your sugggestions and tips. Kudos also to the other mentors/coordinators for organizing this kourses/klassrooms. Ironically though, too bad ksnapshot does not have too many bugs. I think we were literally short of bugs!

Last edited by Linex on Thu Jan 01, 2009 1:25 pm, edited 1 time in total.

Alsagoff, proud to be a member of KDE forums since 2008-Dec.

Fri Jan 02, 2009 11:00 am

RE: Discuss Kourse 1

Fri Jan 02, 2009 11:00 am

Sadly I did not have the time that I could spent that I wanted to because of some circumstances. But nevertheless, I think we managed it very well. Thanks to the students and to the moderators. In a summary we have two patches from (one of them needs just small changes) and a patch to a wish, which is also on a good way as far as I can see. One student is working on detecting some problems with KHotKeys which causes problems in ksnapshot (too many instances) and which also could cause problems in other applications.

But what did we learn from the first kourse? Here are some points which could be improved the next time

Sometimes it was hard for me to follow the thread. Next time each patch should get one thread. I tried it this time but first when I saw that the patch was ready for it
I should have talked with Richard (maintainer of ksnapshot) before. I think would have been a great benefit to have him in the kourse

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Sat Jan 03, 2009 6:27 am

RE: Discuss Kourse 1

Sat Jan 03, 2009 6:27 am

Yes thank you for your work msoeken. And I can't wait for another one from you in due time.

Yeah I think it would really be good for klasses to have the maintainer to help with the mentor and the students. Sorry about my patch >.< My first time hehe, but wish I did a little more digging. But I hope it does well.

Last edited by ComaWhite on Sat Jan 03, 2009 6:27 am, edited 1 time in total.

KDE Version: 4.3.74 | Qt Version: 4.6.0
Aki IRC Developer http://www.akiirc.org/

Sun Jan 04, 2009 6:19 pm

RE: Discuss Kourse 1

Sun Jan 04, 2009 6:19 pm

Unfortunately I hadn't enough time back then when this Kourse was fresh and I wasn't even a student... But nevertheless this whole thing gave me a brilliant insight into KDE development and finally I would like to show you my first patch at this point.
I have played about ComaWhite's solution for the "close KSnapShot when screenshot is opened with another application" (Wish #165482)

Code: Select all: Index: ksnapshot/ksnapshotwidget.ui =================================================================== --- ksnapshot/ksnapshotwidget.ui (Revision 905197) +++ ksnapshot/ksnapshotwidget.ui (Arbeitskopie) @@ -218,6 +218,19 @@ + + + When enabled, KSnapShot will be closed when snapshot is opened with another application + + + Close KSnapshot when opening other application + + + true + + + + Click this button to copy the current snapshot to the clipboard. Index: ksnapshot/ksnapshot.cpp =================================================================== --- ksnapshot/ksnapshot.cpp (Revision 905197) +++ ksnapshot/ksnapshot.cpp (Arbeitskopie) @@ -291,10 +291,14 @@ { return; } - KUrl::List list; list.append(fileopen); - KRun::run(application, list, this); + if ( KRun::run(application, list, this) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + accept(); + } + } void KSnapshot::slotOpen(QAction* action) @@ -330,13 +334,28 @@ if (!service && !dlg.text().isEmpty()) { - KRun::run(dlg.text(), list, this); - return; + if ( KRun::run(dlg.text(), list, this) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + accept(); + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + return; + } + return; } } // we have an action with a service, run it! - KRun::run(*service, list, this, true); + if ( KRun::run(*service, list, this, true) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + accept(); + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + return; + } + } void KSnapshot::slotPopulateOpenMenu() @@ -599,6 +618,11 @@ return mainWidget->cbIncludeDecorations->isChecked(); } +bool KSnapshot::closeWhenOpen() const +{ + return mainWidget->cbCloseWhenOpen->isChecked(); +} + int KSnapshot::mode() const { return mainWidget->comboMode->currentIndex(); Index: ksnapshot/ksnapshot.h =================================================================== --- ksnapshot/ksnapshot.h (Revision 905197) +++ ksnapshot/ksnapshot.h (Arbeitskopie) @@ -182,6 +182,7 @@ void setMode( int mode ); int delay() const; bool includeDecorations() const; + bool closeWhenOpen() const; int mode() const; QPixmap preview(); int previewWidth() const;

There now is a Checkbox right under the "Open with" Button - I think it's the appropriate place for it as long as there are not enought options for a real "options area"...

Last edited by msoeken on Sun Jan 04, 2009 8:44 pm, edited 1 time in total.

michael4910, proud to be a member of KDE forums since 2008-Oct.

Sun Jan 04, 2009 9:13 pm

RE: Discuss Kourse 1

Sun Jan 04, 2009 9:13 pm

michael4910 wrote:I have played about ComaWhite's solution for the "close KSnapShot when screenshot is opened with another application" (Wish #165482)

The patch looks good. But the state of the checkbox should be saved after quitting the program and restored when starting it.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Mon Jan 05, 2009 10:17 pm

RE: Discuss Kourse 1

Mon Jan 05, 2009 10:17 pm

That sounds plausible...
After a while on the KDE API pages, in Qt Assistant and on last.fm I'm somehow satisfied in having gone the next step on my way onto the KDE programming olymp! yeah! ;-)

Here it comes: Kconfig stores the state of the checkbox...

Code: Select all: Index: ../ksnapshot/ksnapshotwidget.ui =================================================================== --- ../ksnapshot/ksnapshotwidget.ui (Revision 905197) +++ ../ksnapshot/ksnapshotwidget.ui (Arbeitskopie) @@ -218,6 +218,19 @@ + + + When enabled, KSnapShot will be closed when snapshot is opened with another application + + + Close KSnapshot when opening other application + + + true + + + + Click this button to copy the current snapshot to the clipboard. Index: ../ksnapshot/ksnapshot.cpp =================================================================== --- ../ksnapshot/ksnapshot.cpp (Revision 905197) +++ ../ksnapshot/ksnapshot.cpp (Arbeitskopie) @@ -162,7 +162,11 @@ setMode( conf.readEntry("mode", 0) ); setIncludeDecorations(conf.readEntry("includeDecorations",true)); filename = KUrl( conf.readPathEntry( "filename", QDir::currentPath()+'/'+i18n("snapshot")+"1.png" )); - + + // shall KSnapshot be closed when "open with" is used? + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + mainWidget->cbCloseWhenOpen->setChecked(confOpen.readEntry("close", false)); + // Make sure the name is not already being used while(KIO::NetAccess::exists( filename, KIO::NetAccess::DestinationSide, this )) { autoincFilename(); @@ -291,10 +295,17 @@ { return; } - KUrl::List list; list.append(fileopen); - KRun::run(application, list, this); + if ( KRun::run(application, list, this) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + confOpen.writeEntry("close",closeWhenOpen()); + confOpen.sync(); + accept(); + } + } void KSnapshot::slotOpen(QAction* action) @@ -330,13 +341,34 @@ if (!service && !dlg.text().isEmpty()) { - KRun::run(dlg.text(), list, this); - return; + if ( KRun::run(dlg.text(), list, this) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + confOpen.writeEntry("close",closeWhenOpen()); + confOpen.sync(); + accept(); + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + return; + } + return; } } // we have an action with a service, run it! - KRun::run(*service, list, this, true); + if ( KRun::run(*service, list, this, true) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + confOpen.writeEntry("close",closeWhenOpen()); + confOpen.sync(); + accept(); + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + return; + } + } void KSnapshot::slotPopulateOpenMenu() @@ -409,8 +441,12 @@ KUrl url = filename; url.setPass( QString::null ); //krazy:exclude=nullstrassign for old broken gcc conf.writePathEntry("filename",url.url()); - conf.sync(); + + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + confOpen.writeEntry("close",closeWhenOpen()); + confOpen.sync(); + e->accept(); } @@ -537,6 +573,9 @@ void KSnapshot::exit() { + KConfigGroup conf(KGlobal::config(), "Open-with settings"); + conf.writeEntry("close",closeWhenOpen()); + conf.sync(); reject(); } @@ -599,6 +638,11 @@ return mainWidget->cbIncludeDecorations->isChecked(); } +bool KSnapshot::closeWhenOpen() const +{ + return mainWidget->cbCloseWhenOpen->isChecked(); +} + int KSnapshot::mode() const { return mainWidget->comboMode->currentIndex(); Index: ../ksnapshot/ksnapshot.h =================================================================== --- ../ksnapshot/ksnapshot.h (Revision 905197) +++ ../ksnapshot/ksnapshot.h (Arbeitskopie) @@ -182,6 +182,7 @@ void setMode( int mode ); int delay() const; bool includeDecorations() const; + bool closeWhenOpen() const; int mode() const; QPixmap preview(); int previewWidth() const;

Last edited by msoeken on Tue Jan 06, 2009 7:05 am, edited 1 time in total.

michael4910, proud to be a member of KDE forums since 2008-Oct.

Tue Jan 06, 2009 7:13 am

RE: Discuss Kourse 1

Tue Jan 06, 2009 7:13 am

Great that you enjoy KDE programming. Loading the option looks ok, but there is some problems with writing.

You write settings, when you open the image with an external application or service
You write settings in the exit() method, which is never called

I prefer to connect the QAbstractButton::toggled(bool) signal to a new slot which writes the settings.

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Fri Jan 09, 2009 7:05 pm

RE: Discuss Kourse 1

Fri Jan 09, 2009 7:05 pm

ahrgh - you're right. Using the toggled-signal is really much better...
Do you still see some space for improvements?

Code: Select all: Index: ../ksnapshot/ksnapshotwidget.ui =================================================================== --- ../ksnapshot/ksnapshotwidget.ui (Revision 905197) +++ ../ksnapshot/ksnapshotwidget.ui (Arbeitskopie) @@ -218,6 +218,19 @@ + + + When enabled, KSnapShot will be closed when snapshot is opened with another application + + + Close KSnapshot when opening other application + + + true + + + + Click this button to copy the current snapshot to the clipboard. Index: ../ksnapshot/ksnapshot.cpp =================================================================== --- ../ksnapshot/ksnapshot.cpp (Revision 906308) +++ ../ksnapshot/ksnapshot.cpp (Arbeitskopie) @@ -100,6 +100,7 @@ connect( mainWidget->btnCopy, SIGNAL( clicked() ), SLOT( slotCopy() ) ); // connect( mainWidget->btnOpen, SIGNAL( clicked() ), SLOT( slotOpen() ) ); connect( mainWidget->comboMode, SIGNAL( activated(int) ), SLOT( slotModeChanged(int) ) ); + connect( mainWidget->cbCloseWhenOpen, SIGNAL( toggled(bool) ), SLOT( slotCloseWhenOpenToggle(bool) )); openMenu = new QMenu(this); mainWidget->btnOpen->setMenu(openMenu); @@ -162,7 +163,11 @@ setMode( conf.readEntry("mode", 0) ); setIncludeDecorations(conf.readEntry("includeDecorations",true)); filename = KUrl( conf.readPathEntry( "filename", QDir::currentPath()+'/'+i18n("snapshot")+"1.png" )); - + + // shall KSnapshot be closed when "open with" is used? + KConfigGroup confOpen(KGlobal::config(), "Open-with settings"); + mainWidget->cbCloseWhenOpen->setChecked(confOpen.readEntry("close", false)); + // Make sure the name is not already being used while(KIO::NetAccess::exists( filename, KIO::NetAccess::DestinationSide, this )) { autoincFilename(); @@ -294,7 +299,12 @@ KUrl::List list; list.append(fileopen); - KRun::run(application, list, this); + if ( KRun::run(application, list, this) && closeWhenOpen() ) { + // if the service was able to run then we silently + // close the application. + accept(); + } + } void KSnapshot::slotOpen(QAction* action) @@ -330,13 +340,31 @@ if (!service && !dlg.text().isEmpty()) { - KRun::run(dlg.text(), list, this); - return; + if ( KRun::run(dlg.text(), list, this) ) { + // if the service was able to run then we silently + // close the application. + if ( closeWhenOpen() ){ + accept(); + } + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + } + return; } } // we have an action with a service, run it! - KRun::run(*service, list, this, true); + if ( KRun::run(*service, list, this, true) ) { + // if the service was able to run then we silently + // close the application. + if ( closeWhenOpen() ){ + accept(); + } + } else { + KMessageBox::error(this, i18n("Unable to open %1. Please check to make sure the application was properly installed.", service->name())); + return; + } + } void KSnapshot::slotPopulateOpenMenu() @@ -400,6 +428,13 @@ show(); } +void KSnapshot::slotCloseWhenOpenToggle(bool checked) +{ + KConfigGroup conf(KGlobal::config(), "Open-with settings"); + conf.writeEntry("close", checked); + conf.sync(); +} + void KSnapshot::closeEvent( QCloseEvent * e ) { KConfigGroup conf(KGlobal::config(), "GENERAL"); @@ -599,6 +634,11 @@ return mainWidget->cbIncludeDecorations->isChecked(); } +bool KSnapshot::closeWhenOpen() const +{ + return mainWidget->cbCloseWhenOpen->isChecked(); +} + int KSnapshot::mode() const { return mainWidget->comboMode->currentIndex(); Index: ../ksnapshot/ksnapshot.h =================================================================== --- ../ksnapshot/ksnapshot.h (Revision 905197) +++ ../ksnapshot/ksnapshot.h (Arbeitskopie) @@ -157,6 +157,7 @@ void setTime( int newTime ); void setURL( const QString &newURL ); void setGrabMode( int m ); + void slotCloseWhenOpenToggle( bool checked ); void exit(); protected: @@ -182,6 +183,7 @@ void setMode( int mode ); int delay() const; bool includeDecorations() const; + bool closeWhenOpen() const; int mode() const; QPixmap preview(); int previewWidth() const;

Last edited by michael4910 on Fri Jan 09, 2009 7:07 pm, edited 1 time in total.

michael4910, proud to be a member of KDE forums since 2008-Oct.

Fri Jan 09, 2009 7:13 pm

RE: Discuss Kourse 1

Fri Jan 09, 2009 7:13 pm

michael4910 wrote:ahrgh - you're right. Using the toggled-signal is really much better...
Do you still see some space for improvements?

Looks very good. If you want to, you can send the patch to the kdegraphics mailing list or send it to the maintainer Richard (you will find his email in the about dialog). Feature freeze is over and so it could go directly into trunk. If you have further questions, feel free to ask.

Cheers, m

[size=x-small]code | [url=cia.vc/stats/author/msoeken]cia.vc[/url] | [url=kde.org/support]donating KDE[/url] | [url=tinyurl.com/cto4ns]wishlist[/url][/size]

Who is online