Differential D23534

FIX: Properly close spectacle after Save As
ClosedPublic
Actions

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

Details

Reviewers
davidre
Commits
R166:e98249c30917: FIX: Properly close spectacle after Save As
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.Aug 28 2019, 11:44 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 28 2019, 11:44 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
aprcela requested review of this revision.Aug 28 2019, 11:44 AM
davidre added inline comments.Aug 28 2019, 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)Aug 28 2019, 2:00 PM
davidre added a comment.Aug 28 2019, 2:25 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.Aug 28 2019, 2:25 PM
aprcela updated this revision to Diff 64885.Aug 28 2019, 7:28 PM
  • Remove redundant check and redundant connect
aprcela marked 2 inline comments as done.Aug 28 2019, 7:32 PM
In D23534#521136, @davidre wrote:

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.Sep 1 2019, 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.Sep 1 2019, 6:58 PM
In D23534#523735, @davidre wrote:

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) .

davidre added a comment.Sep 1 2019, 7:39 PM
In D23534#523826, @aprcela wrote:
In D23534#523735, @davidre wrote:

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.EditedSep 1 2019, 7:46 PM
In D23534#523840, @davidre wrote:
In D23534#523826, @aprcela wrote:
In D23534#523735, @davidre wrote:

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)

davidre added a comment.Sep 2 2019, 3:25 PM

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.Sep 2 2019, 7:11 PM
  • Revert connect and change if condition inside it
aprcela added a comment.Sep 2 2019, 7:12 PM
In D23534#524562, @davidre wrote:

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.EditedSep 3 2019, 10:16 AM

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

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

Ok!

Closed by commit R166:e98249c30917: FIX: Properly close spectacle after Save As (authored by aprcela, committed by davidre). · Explain WhySep 3 2019, 2:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/SpectacleCore.cpp (4 lines)

Diff 65326

View Options

src/SpectacleCore.cpp

Loading...