[Kourse 1] Fixing ksnapshot bugs

[msoeken]

[Kourse 1] Fixing ksnapshot bugs

Wed Nov 26, 2008 7:19 am

[size=x-large] Fixing ksnapshot bugs[/size]
For this kourse I am looking for five students who want to work with me on fixing five ksnapshot bugs. The fixes should go into upcoming KDE 4.2 release. My plan is that each of the five students is working on one bug. If it does not fit, it is also ok. This is my first try to prepare a kourse for the klassroom. You should be fine with C++ to read the code. Questions about Qt4 and KDE4 specific issues can be discussed here. But if you have no experience with Qt4/KDE4 you should take a look at the tutorials at Trolltech and KDE. If you have no experience in programming, you can help with investigating some of these bugs or other bugs in ksnapshot and discuss them here and on bugs.kde.org.

[size=medium]Why ksnapshot?[/size]
I choose ksnapshot, because it is a comparatively small application. It is often very difficult to understand the code from an other project. With this project you should get an overview quickly.

[size=x-large] Preparation[/size]
[size=medium]If you are using <= KDE 4.1.3[/size]

1. Follow the instructions from Techbase to build KDE 4.2 from trunk. You should do this with an extra user, e.g. kde-devel.
2. Run ./kdesvn-build kdegraphics from your ~/kdesvn directory.
3. You can find the ksnapshot code in ~/kdesvn/kdegraphics/ksnapshot.

[size=medium]If you are using KDE 4.2 Beta 2[/size]

1. You can checkout kdegraphics from trunk with your current user:
   

   Code: Select all

   svn co svn://anonsvn.kde.org/home/kde/trunk/KDE/kdegraphics

2. Change into the directory kdegraphics and create a directory build
   

   Code: Select all

   mkdir build

3. Change into the directory build and run cmake there
   

   Code: Select all

   cmake ..

4. Change into the directory ksnapshot and run make there
   

   Code: Select all

   make

5. The code to work with is in kdegraphics/ksnapshot and not in the build directory.

[size=x-large] Picking a bug[/size]
Choose one or more from the following five bugs you want to work on. Read the description and the comments on the bko (bugs.kde.org) page and try to reproduce it with the ksnapshot version from trunk. You should also register at bko to be able to write comments.

1. Bug 174181: Can't exit of KSnapshot zone selection
   Bug 160880: Alt-Tab while in region capture mode prevents exit
   These bugs seems to be duplicates. You could check this and write an appropriate comment.
2. Bug 106679: KSnapshot should offer anyname010.png after saving anyname009.png
   There are other counting issues in the comments of this bug report.
3. Bug 166190: ksnapshot Infinite fork bug
   This could be solved by disabling multiple instances of ksnapshot
4. Bug 136142: KSnapshot won't take screenshots of narrow windows
5. Bug 175186: Timer rectangle is too small

Because some of these bugs seems already been fixed during the period of writing and publishing this kourse, here are some wishes which are also interesting. But because of hard feature freeze, they could first be released for KDE 4.4.

1. Wish 91881: Take multiple screenshots at certain intervals
2. Wish 166608: PrintScreen button should open KSnapshot (it does in KDE 4.0.4 but not in 4.1)
   This seems to be a bug and not a wish.
3. Wish 165482: After opening with "Open with...", Ksnapshot should self-close
4. Wish 158677: Dragging screenshot to a text box should save to temporary file and insert the file name

So, now it is your turn. Choose a bug and write a response to this thread. Try to write a patch and send it to me. I will have a look on it and if it looks ok, it can be committed and the bug can be closed. Happy hacking!

Last edited by sayakb on Fri Jan 02, 2009 5:48 pm, edited 1 time in total.

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 4:26 pm

I'll take #2:
Bug 106679: KSnapshot should offer anyname010.png after saving anyname009.png
There are other counting issues in the comments of this bug report.

EDIT:
Found a function that does the renaming

void KSnapshotObject::autoincFilename()

Now the hard part, the logic behind renaming [hr]
Well, it seems that most of the bug had already been fixed.
The only thing that can possibly be a bug is a transition from name09 → name10 (or name099 → name100).
Shall I try to fix this?

Last edited by jernejovc on Sat Dec 27, 2008 4:51 pm, edited 1 time in total.

[neverendingo]

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 6:10 pm

Seems to work already, ksnapshot offers name10 after name09.

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 6:21 pm

Sorry, I meant name09→name010 (if there is 0 before the number 0 stays).

[sayakb]

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 6:25 pm

Here, I get name09 -> name10. So it looks fixed. And so is #3 and #4. We'll try to come up with more valid ones.

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 7:15 pm

patch for "5. Bug 175186: Timer rectangle is too small"

Code: Select all

Index: snapshottimer.cpp
===================================================================
--- snapshottimer.cpp   (revision 902189)
+++ snapshottimer.cpp   (working copy)
@@ -31,7 +31,7 @@
 SnapshotTimer::SnapshotTimer() : QWidget(0)
 {
     setWindowFlags( Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::X11BypassWindowManagerHint);
-    resize(180,20);
+    resize(220,20);
     connect(&timer, SIGNAL(timeout()), this, SLOT(bell()));
 }


Last edited by dhanasekar on Sat Dec 27, 2008 7:17 pm, edited 1 time in total.

[ComaWhite]

RE: [Klassroom] Fixing ksnapshot bugs

Sat Dec 27, 2008 9:40 pm

okay I'll take this one
Bug 174181: Can't exit of KSnapshot zone selection
Bug 160880: Alt-Tab while in region capture mode prevents exit

Can't exit out of KSnapshot zone selection even if you don't alt-tab. Tested it and had to alt+ctrl+del to get out of it

Edit:Apparently 174181 is invalid. Because when I'm selected region it accepts the snapshot perfect. However when I perform bug 160880 that does happen. The only way to get out I found is just to double click the mouse a heck of alot and it will break out of it selecting some borders edge. But I'm not able to reproduce 174181 with the latest svn (4.1.86).

Last edited by ComaWhite on Sat Dec 27, 2008 10:11 pm, edited 1 time in total.

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 2:14 am

alternative patch for "5. Bug 175186: Timer rectangle is too small"

ok I looked at dhanasekar's patch but its a rather simplistic solution.
1) The resize's width is still hardcoded, it was simply given a higher number.
--a) thus desktops with bigger fonts may still not see 'seconds'.
--b) you never know the width of the foreign language text it might be translated to, so ...

2) The text is set to wrap(from the original code) but the height is fixed. This patch set it to Qt::TextSingleLine. At least part of the word will show even if the width is not wide enough or else the whole 'seconds' word will not show. (may not matter now)

3) Tested this patch with big wide fonts and seems to work each time. Patch resizes not only the width but the height of the text also (no potential bug now).

Code: Select all

Index: snapshottimer.cpp
===================================================================
--- snapshottimer.cpp   (revision 902256)
+++ snapshottimer.cpp   (working copy)
@@ -31,7 +31,8 @@
 SnapshotTimer::SnapshotTimer() : QWidget(0)
 {
     setWindowFlags( Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::X11BypassWindowManagerHint);
-    resize(180,20);
+    pluralText = "Snapshot will be taken in %1 seconds";
+    resize(fontMetrics().width(pluralText),fontMetrics().height());
     connect(&timer, SIGNAL(timeout()), this, SLOT(bell()));
 }

@@ -85,8 +86,8 @@
       painter.setPen( textColor );
       painter.setBrush( textBackgroundColor );
       QString helpText = i18np( "Snapshot will be taken in 1 second",
-                                "Snapshot will be taken in %1 seconds", ( length-time ) );
-      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextWordWrap, helpText );
+                                pluralText, ( length-time ) );
+      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextSingleLine, helpText );
       textRect.adjust( -2, -2, 4, 2 );
       painter.drawRect( rect().adjusted(0,0,-1,-1) );
       textRect.moveTopLeft( QPoint( 3, 3 ) );
Index: snapshottimer.h
===================================================================
--- snapshottimer.h     (revision 902256)
+++ snapshottimer.h     (working copy)
@@ -44,6 +44,7 @@
     void paintEvent( QPaintEvent* e );

   private:
+    QByteArray pluralText;
     QTimer timer;
     int time;
     int length;


Last edited by Linex on Sun Dec 28, 2008 2:23 am, edited 1 time in total.

[ComaWhite]

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 8:45 am

Hmm I've been at the first bug for over a few hours. And I've tried handling

the eventFilter on QEvent::ActivationChanged. I've tried always keeping focus, or when lost even back focus. I've tried it in many places. And still won't give focus back. The only way we to ignore Tab/Alt but thats a cheap hack to fix it. But I'm really stumped on it.

[msoeken]

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 11:21 am

alternative patch for "5. Bug 175186: Timer rectangle is too small"

ok I looked at dhanasekar's patch but its a rather simplistic solution.
1) The resize's width is still hardcoded, it was simply given a higher number.
--a) thus desktops with bigger fonts may still not see 'seconds'.
--b) you never know the width of the foreign language text it might be translated to, so ...

2) The text is set to wrap(from the original code) but the height is fixed. This patch set it to Qt::TextSingleLine. At least part of the word will show even if the width is not wide enough or else the whole 'seconds' word will not show. (may not matter now)

3) Tested this patch with big wide fonts and seems to work each time. Patch resizes not only the width but the height of the text also (no potential bug now).

Code: Select all

Index: snapshottimer.cpp
===================================================================
--- snapshottimer.cpp   (revision 902256)
+++ snapshottimer.cpp   (working copy)
@@ -31,7 +31,8 @@
 SnapshotTimer::SnapshotTimer() : QWidget(0)
 {
     setWindowFlags( Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::X11BypassWindowManagerHint);
-    resize(180,20);
+    pluralText = "Snapshot will be taken in %1 seconds";
+    resize(fontMetrics().width(pluralText),fontMetrics().height());
     connect(&timer, SIGNAL(timeout()), this, SLOT(bell()));
 }

@@ -85,8 +86,8 @@
       painter.setPen( textColor );
       painter.setBrush( textBackgroundColor );
       QString helpText = i18np( "Snapshot will be taken in 1 second",
-                                "Snapshot will be taken in %1 seconds", ( length-time ) );
-      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextWordWrap, helpText );
+                                pluralText, ( length-time ) );
+      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextSingleLine, helpText );
       textRect.adjust( -2, -2, 4, 2 );
       painter.drawRect( rect().adjusted(0,0,-1,-1) );
       textRect.moveTopLeft( QPoint( 3, 3 ) );
Index: snapshottimer.h
===================================================================
--- snapshottimer.h     (revision 902256)
+++ snapshottimer.h     (working copy)
@@ -44,6 +44,7 @@
     void paintEvent( QPaintEvent* e );

   private:
+    QByteArray pluralText;
     QTimer timer;
     int time;
     int length;


That looks good. But you should use an i18n call to translate the pluralText in the constructor.[hr]

Hmm I've been at the first bug for over a few hours. And I've tried handling

the eventFilter on QEvent::ActivationChanged. I've tried always keeping focus, or when lost even back focus. I've tried it in many places. And still won't give focus back. The only way we to ignore Tab/Alt but thats a cheap hack to fix it. But I'm really stumped on it.

Couldn't we stop snapshotting when the user presses ALT-Tab. What could be a reasen for the user to switch windows while taking a snapshot.[hr]

Here, I get name09 -> name10. So it looks fixed. And so is #3 and #4. We'll try to come up with more valid ones.

How could you reproduce #3. I thought the hotkey subsystem does not work with KDE 4.2. Another question for #3 is, whether it is useful to have several instances. Because to have several instances could be error prone.[hr]
Because some bugs have already been fixed, I added some wishes which could be implemented for KDE 4.4.

Last edited by msoeken on Sun Dec 28, 2008 11:36 am, edited 1 time in total.

[ComaWhite]

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 12:28 pm

Hmm I've been at the first bug for over a few hours. And I've tried handling

the eventFilter on QEvent::ActivationChanged. I've tried always keeping focus, or when lost even back focus. I've tried it in many places. And still won't give focus back. The only way we to ignore Tab/Alt but thats a cheap hack to fix it. But I'm really stumped on it.

Couldn't we stop snapshotting when the user presses ALT-Tab. What could be a reasen for the user to switch windows while taking a snapshot.[hr]

Well also there should really be no reason to alt-tab when doing region selection anyways or even screen capturing period imho. But I don't want to code something and it not be the way the developers want their project done that way.

Edit: Missing quote bb end tag

Last edited by ComaWhite on Sun Dec 28, 2008 12:30 pm, edited 1 time in total.

[msoeken]

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 6:12 pm

Hmm I've been at the first bug for over a few hours. And I've tried handling

the eventFilter on QEvent::ActivationChanged. I've tried always keeping focus, or when lost even back focus. I've tried it in many places. And still won't give focus back. The only way we to ignore Tab/Alt but thats a cheap hack to fix it. But I'm really stumped on it.

Couldn't we stop snapshotting when the user presses ALT-Tab. What could be a reasen for the user to switch windows while taking a snapshot.[hr]

Well also there should really be no reason to alt-tab when doing region selection anyways or even screen capturing period imho. But I don't want to code something and it not be the way the developers want their project done that way.

Edit: Missing quote bb end tag

Then this should be discussed either in bugs.kde.org or with Richard, the maintainer, directly. I will write him an email. Perhaps he is also interested in joining this thread.

RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 7:49 pm

alternative patch for "5. Bug 175186: Timer rectangle is too small"

ok I looked at dhanasekar's patch but its a rather simplistic solution.
1) The resize's width is still hardcoded, it was simply given a higher number.
--a) thus desktops with bigger fonts may still not see 'seconds'.
--b) you never know the width of the foreign language text it might be translated to, so ...

2) The text is set to wrap(from the original code) but the height is fixed. This patch set it to Qt::TextSingleLine. At least part of the word will show even if the width is not wide enough or else the whole 'seconds' word will not show. (may not matter now)

3) Tested this patch with big wide fonts and seems to work each time. Patch resizes not only the width but the height of the text also (no potential bug now).

Code: Select all

Index: snapshottimer.cpp
===================================================================
--- snapshottimer.cpp   (revision 902256)
+++ snapshottimer.cpp   (working copy)
@@ -31,7 +31,8 @@
 SnapshotTimer::SnapshotTimer() : QWidget(0)
 {
     setWindowFlags( Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::X11BypassWindowManagerHint);
-    resize(180,20);
+    pluralText = "Snapshot will be taken in %1 seconds";
+    resize(fontMetrics().width(pluralText),fontMetrics().height());
     connect(&timer, SIGNAL(timeout()), this, SLOT(bell()));
 }

@@ -85,8 +86,8 @@
       painter.setPen( textColor );
       painter.setBrush( textBackgroundColor );
       QString helpText = i18np( "Snapshot will be taken in 1 second",
-                                "Snapshot will be taken in %1 seconds", ( length-time ) );
-      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextWordWrap, helpText );
+                                pluralText, ( length-time ) );
+      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextSingleLine, helpText );
       textRect.adjust( -2, -2, 4, 2 );
       painter.drawRect( rect().adjusted(0,0,-1,-1) );
       textRect.moveTopLeft( QPoint( 3, 3 ) );
Index: snapshottimer.h
===================================================================
--- snapshottimer.h     (revision 902256)
+++ snapshottimer.h     (working copy)
@@ -44,6 +44,7 @@
     void paintEvent( QPaintEvent* e );

   private:
+    QByteArray pluralText;
     QTimer timer;
     int time;
     int length;


That looks good. But you should use an i18n call to translate the pluralText in the constructor.[hr]

ok, I use an i18n call to translate the pluralText in the constructor. Since paintEvent() below will use pluralText, I only i18n it in resize(). it Here is the revised patch.

Code: Select all

Index: snapshottimer.cpp
===================================================================
--- snapshottimer.cpp   (revision 902256)
+++ snapshottimer.cpp   (working copy)
@@ -31,7 +31,8 @@
 SnapshotTimer::SnapshotTimer() : QWidget(0)
 {
     setWindowFlags( Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::X11BypassWindowManagerHint);
-    resize(180,20);
+    pluralText = "Snapshot will be taken in %1 seconds";
+    resize(fontMetrics().width(i18n(pluralText)),fontMetrics().height());
     connect(&timer, SIGNAL(timeout()), this, SLOT(bell()));
 }

@@ -85,8 +86,8 @@
       painter.setPen( textColor );
       painter.setBrush( textBackgroundColor );
       QString helpText = i18np( "Snapshot will be taken in 1 second",
-                                "Snapshot will be taken in %1 seconds", ( length-time ) );
-      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextWordWrap, helpText );
+                                pluralText, ( length-time ) );
+      QRect textRect = painter.boundingRect( rect().adjusted( 2, 2, -2, -2 ), Qt::TextSingleLine, helpText );
       textRect.adjust( -2, -2, 4, 2 );
       painter.drawRect( rect().adjusted(0,0,-1,-1) );
       textRect.moveTopLeft( QPoint( 3, 3 ) );
Index: snapshottimer.h
===================================================================
--- snapshottimer.h     (revision 902256)
+++ snapshottimer.h     (working copy)
@@ -44,6 +44,7 @@
     void paintEvent( QPaintEvent* e );

   private:
+    QByteArray pluralText;
     QTimer timer;
     int time;
     int length;


RE: [Klassroom] Fixing ksnapshot bugs

Sun Dec 28, 2008 8:42 pm

Ok, I'll take the infinite fork loop bug. (which isn't really a problem with forking processes)

Function keys in general work the same as ordinary letters - even in non-KDE applications like Firefox. Push the print-screen button and hold it to get potentially hundreds of ksnapshot instances, same with F1 and the firefox help page, etc. It's a common problem that should be IMHO solved upstream.

There were several bugs in X.org/kernel that caused normal keys to get stuck in the 'down' position. These could wery well be contributing to the problem (or have been, I can't trigger this anymore). I remember having a similar problem with DosBox a few months back.

Some possible fixes:

• Force ksnapshot to be a single-instance application... which IMHO won't solve the real problem but rather hide it.
• Make KHotKeys detect autorepeat events and add an option to not react to those. Set ksnapshot (and possibly others) to use this by default.

I'll try doing both

[ComaWhite]

RE: [Klassroom] Fixing ksnapshot bugs

Mon Dec 29, 2008 12:21 am

Hmm I've been at the first bug for over a few hours. And I've tried handling

the eventFilter on QEvent::ActivationChanged. I've tried always keeping focus, or when lost even back focus. I've tried it in many places. And still won't give focus back. The only way we to ignore Tab/Alt but thats a cheap hack to fix it. But I'm really stumped on it.

Couldn't we stop snapshotting when the user presses ALT-Tab. What could be a reasen for the user to switch windows while taking a snapshot.[hr]

Well also there should really be no reason to alt-tab when doing region selection anyways or even screen capturing period imho. But I don't want to code something and it not be the way the developers want their project done that way.

Edit: Missing quote bb end tag

Then this should be discussed either in bugs.kde.org or with Richard, the maintainer, directly. I will write him an email. Perhaps he is also interested in joining this thread.

okay I figured out why. I documented it in comments so it explains why and what not.

Code: Select all

--- regiongrabber.cpp  2008-12-28 18:23:27.000000000 -0600
+++ regiongrabber.cpp   2008-12-28 18:23:07.000000000 -0600
@@ -213,6 +213,17 @@ void RegionGrabber::mousePressEvent( QMo

 void RegionGrabber::mouseMoveEvent( QMouseEvent* e )
 {
+   // If user calls tab-alt that normally switches the window focus.
+   // The way the window is set up on X11 causes it not to handle
+   // inputs once focus is lost so we need to call it when the
+   // mouse is moved again. Probably not the best way to do this but
+   // only way I figured to handle it properly.
+#if defined(Q_WS_X11)
+   // Activate the window and raise it to the top of the focus list
+   activateWindow();
+   raise();
+#endif
+
     if ( mouseDown )
     {
         if ( newSelection )


Last edited by ComaWhite on Mon Dec 29, 2008 6:38 am, edited 1 time in total.