Here is for the Wish #3.
Sorry if my patches are wrong.

Code: Select all

--- ksnapshot.cpp   2008-12-29 00:39:33.000000000 -0600
+++ ksnapshot.cpp   2008-12-29 00:38:53.000000000 -0600
@@ -336,7 +336,14 @@ void KSnapshot::slotOpen(QAction* action
     }
 
     // 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.
+      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()


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.

Shouldn't this be done higher up, like in khotkeys ?

Wish 165482: After opening with "Open with...", Ksnapshot should self-close

This wish might not be a good idea. What if I want to open the screenshot in another program, oops already closed. I think it is fine as it is.

Wish 166608 has already been fixed, thanks to the KHotkeys refactor for 4.2.

Nothing was wrong with KSnapshot in 4.1 with regard to that functionality, it was just never called because KHotkeys was broken.

Linex wrote: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;


Looks good. I will patch my working copy and check it again then. If all works correctly it can be comitted. Thanks.[hr]

ComaWhite wrote: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 )


This could be a solution. But I am not able to say that this is really right and if this is ok with the maintainer. I think this patch should not be comitted directly, but added as an attachment to the bugs comments.[hr]

ComaWhite wrote:Here is for the Wish #3.
Sorry if my patches are wrong.

Code: Select all

--- ksnapshot.cpp   2008-12-29 00:39:33.000000000 -0600
+++ ksnapshot.cpp   2008-12-29 00:38:53.000000000 -0600
@@ -336,7 +336,14 @@ void KSnapshot::slotOpen(QAction* action
     }
 
     // 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.
+      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()


This looks good, but the bug report stated, that this feature should be configurable. Have a look at http://techbase.kde.org/Development/Tutorials/Using_KConfig_XT. If you want to, you can also thought about other things which can go into a config dialog, because there is no one at moment. The "Close after Open As" would be the single option in a dialog.

Well if there is only one option. Then it should go under the other checkbox. But I tried doing that in the designer and it totally screwed up the layout. It needs to be redesigned with a grid dragged onto it. And then expanded into a grid on the main window. Because right now its hard to add to it.

ComaWhite wrote:Well if there is only one option. Then it should go under the other checkbox. But I tried doing that in the designer and it totally screwed up the layout. It needs to be redesigned with a grid dragged onto it. And then expanded into a grid on the main window. Because right now its hard to add to it.

You can just drag a new checkbox below the other in the designer. I do not think you have to reimplement it. But I am not sure whether this is a good place for the option. But nevertheless you cut put it there in your first patch and implement the functionality. Later it could be put elsewhere, if this is whished.

ComaWhite wrote:

msoeken wrote:

ComaWhite wrote:

msoeken wrote:

ComaWhite wrote: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 )


The thing with this patch is that the user needs to move the mouse after he pressed Alt-Tab but how is he to know this ? Thus, this small modfication. Not sure if we only need this on X11, so I have left that out. All credit goes to Comawhite.

Code: Select all

Index: regiongrabber.cpp
===================================================================
--- regiongrabber.cpp   (revision 904245)
+++ regiongrabber.cpp   (working copy)
@@ -182,6 +182,17 @@
     selection = r;
 }

+bool RegionGrabber::event( QEvent* e )
+{
+  if(e->type() == QEvent::WindowDeactivate)
+  {
+    activateWindow();
+    raise();
+    return true;
+  }
+  return QWidget::event( e );
+}
+
 void RegionGrabber::mousePressEvent( QMouseEvent* e )
 {
     showHelp = false;
Index: regiongrabber.h
===================================================================
--- regiongrabber.h     (revision 904245)
+++ regiongrabber.h     (working copy)
@@ -45,6 +45,7 @@
     void regionGrabbed( const QPixmap & );

 protected:
+    bool event( QEvent* e );
     void paintEvent( QPaintEvent* e );
     void resizeEvent( QResizeEvent* e );
     void mousePressEvent( QMouseEvent* e );


peterix wrote: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

That sounds good. But instead of detecting autorepeat, KHotKeys should distinguish between KeyUp and KeyDown Events. When listening for KeyUp you are not able to trigger it twice during one press on the key. I am not sure whether listening for KeyDown does make sense but I do not know every use case.

Ok, this was the first kourse. Thanks to all students and helping hands. In summary, four bugs have got attention with great results. One patch has already been sent to the maintainer and two other patches just needs some small modifications. One bug brought attention to some problems in KHotKeys with autorepeat events. I am now closing this thread, but we can still talk about this kourse in the discussion thread.

Cheers, m