FIX: Properly close spectacle after Save As
ClosedPublic

Authored by aprcela on Wed, Aug 28, 11:44 AM.

Details

Summary

BUG: 389694

Test Plan
  1. run Spectacle via shell
  2. Have Quit after Save or Copy checked.
  3. Hit Save As and save the image
  4. Spectacle closes

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aprcela created this revision.Wed, Aug 28, 11:44 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptWed, Aug 28, 11:44 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
aprcela requested review of this revision.Wed, Aug 28, 11:44 AM
davidre added inline comments.Wed, Aug 28, 1:38 PM
src/SpectacleCore.cpp
353

Do we need this here, too?

363

Can be simplified to mStartMode != StartMode::Gui || SpectacleConfig::instance()->quitAfterSaveOrCopyChecked()

ngraham edited the summary of this revision. (Show Details)Wed, Aug 28, 2:00 PM

BUG: 389694

I don't know if this fixes the bug completely. If you read the comments there are multiple ways described to cause spectacle to stay open.
Maybe rather CCBUG?

davidre retitled this revision from [spectacle] FIX: Properly close spectacle after Save As to FIX: Properly close spectacle after Save As.Wed, Aug 28, 2:25 PM
aprcela updated this revision to Diff 64885.Wed, Aug 28, 7:28 PM
  • Remove redundant check and redundant connect
aprcela marked 2 inline comments as done.Wed, Aug 28, 7:32 PM

BUG: 389694

I don't know if this fixes the bug completely. If you read the comments there are multiple ways described to cause spectacle to stay open.
Maybe rather CCBUG?

I just tested the stuff that's mentioned in that bug.
With this approach, it works for every case that I can see there:

  • Proper close on Copy to clipboard
  • Proper close on Save As including proper close for: click on the thumbnail in the notification (opens default image viewer), wait until notification disappears, close notification, click on the notification options and change settings
src/SpectacleCore.cpp
353

Tested, works without.

363

Done. Thanks !

davidre added a subscriber: broulik.Sun, Sep 1, 5:25 PM

Curious that it seems to work without it. @broulik is setting the URL enough for clicking it to open the image? Is this handled by KNotification then? Also will this only work in Plasma or would the connect still be needed for when not under Plasma?

src/SpectacleCore.cpp
359

I was not referencing the whole connect but if we need the other condition too.

aprcela marked 2 inline comments as done.Sun, Sep 1, 6:58 PM

Curious that it seems to work without it. @broulik is setting the URL enough for clicking it to open the image? Is this handled by KNotification then? Also will this only work in Plasma or would the connect still be needed for when not under Plasma?

I tested only on plasma. If Spectacle is launched this way: `spectacle -b' , than it saves the image to the default location and shows the notification. The thumbnail in the notification appears after a very short time tho (less than half a sec) .

Curious that it seems to work without it. @broulik is setting the URL enough for clicking it to open the image? Is this handled by KNotification then? Also will this only work in Plasma or would the connect still be needed for when not under Plasma?

I tested only on plasma. If Spectacle is launched this way: `spectacle -b' , than it saves the image to the default location and shows the notification. The thumbnail in the notification appears after a very short time tho (less than half a sec) .

It's about opening the picture by clicking on the notification.

aprcela added a comment.EditedSun, Sep 1, 7:46 PM

Curious that it seems to work without it. @broulik is setting the URL enough for clicking it to open the image? Is this handled by KNotification then? Also will this only work in Plasma or would the connect still be needed for when not under Plasma?

I tested only on plasma. If Spectacle is launched this way: `spectacle -b' , than it saves the image to the default location and shows the notification. The thumbnail in the notification appears after a very short time tho (less than half a sec) .

It's about opening the picture by clicking on the notification.

Oh, that. Works fine (in Plasma)

I talked with Kai and we need the connection in line 341 for not plasma implementations of notifications. So best to restore it and also update the condition there.

aprcela updated this revision to Diff 65269.Mon, Sep 2, 7:11 PM
  • Revert connect and change if condition inside it

I talked with Kai and we need the connection in line 341 for not plasma implementations of notifications. So best to restore it and also update the condition there.

Like this?

davidre accepted this revision.EditedTue, Sep 3, 10:16 AM

I think that's fine. Should also go to stable

This revision is now accepted and ready to land.Tue, Sep 3, 10:16 AM
aprcela marked 3 inline comments as done.Tue, Sep 3, 10:57 AM

Ok!

This revision was automatically updated to reflect the committed changes.