In D11203#273352, @rkflx wrote:

Fix "Open" button in screenshot notification not responding to click

I don't really understand how to reproduce the problem from your test plan. Could you add detailed steps to reproduce, so I can check how it works before and after your patch? Ideally in terms of Spectacle's command line arguments, which should be similar to the global shortcuts.

@rkflx i think someone else should take over

In D9117#237297, @rkflx wrote:

In D9117#237296, @kapillamba4 wrote:

I think i will add notification in https://phabricator.kde.org/D11203 (a checkbox in configure menu of spectacle to toggle show/hide notification on copying to clipboard) after this revision

In that case it would be best to create a new Diff entirely. Also, please don't add a checkbox to Spectacle. As I wrote above, the official way to configure this is via the existing button on the notification (which opens the respective KCM).

In D9117#237295, @rkflx wrote:

@kapillamba4 Are you still planning to add some sort of notification, or should I review the code now? (Won't be done before next week anyway…)

Tested all shortcuts

Format Code and update patch to work with latest commit b50dc95

Changed commit message

Open button does not work sometimes for Spectacle v18.03.70:

@rkflx, Added kconf_update :)

In D9117#211878, @rkflx wrote:

As for the update mechanism, I found https://forum.kde.org/viewtopic.php?f=17&t=88836. Could you have a look? Perhaps there is an official way we are supposed to use for this.

In D9117#209935, @ngraham wrote:

FWIW, I also do not get the new hotkeys on upgrade, even when I deploy everything to /usr and then reboot. This needs to be fixed or else current users won't get the new feature.

In D9117#208821, @rkflx wrote:

In D9117#208750, @rkflx wrote:

Somehow there's still a bug. If I make install to /usr from the master branch, Print will call the Spectacle GUI just fine. However with your patch, kded will only print the usual DBus message without the GUI appearing and I have to killall spectacle afterwards.

Can anybody reproduce?

@rkflx, There is no need to import spectacle.khotkeys file seperately, Spectacle will install spectacle.khotkeys into /usr/share/khotkeys upon installation itself.

@rkflx, what do you think about adding a static variable inside takeNewScreenshotDBus() and takeNewScreenshot() methods to keep track of how many times these methods are called and prevent this D9117#202850 problem

@rkflx, I didn't update DataCount in spectacle.khotkeys that's why you are not seeing the new shortcuts. I'll just update this patch. You will need to import the spectacle.khotkeys file to get the new shortcuts in kcmshell5 khotkeys

In D9117#208026, @rkflx wrote:

@kapillamba4 Thanks, I'll look at it in a bit. Are you still planning to open a new Diff against Klipper, or should someone else?

In D9117#202433, @ngraham wrote:

In D9117#202396, @kapillamba4 wrote:

wow, it works. Didn't knew that klipper had this option. Should have read the klipper handbook first.
Thanks @rkflx, all six shortcuts work now, no need to use any external program. We should change the default checked state in klipper settings for 'ignore images' to unchecked.

Sounds great, and I would approve of that. Could you open another revision to do this? Thanks!

By multiple connections do you mean multiple instance of the application? If yes then we can change the StartupOption for KDBusService, but i don't see any of these 6 shortcuts creating multiple instance of this application.

In D9117#202346, @rkflx wrote:

@kapillamba4 If I uncheck "Ignore images" in Klipper's settings (it's checked by default, but why?), clicking on Copy To Clipboard and closing Spectacle afterwards allows me to paste the screenshot.

Could you check whether it works then with your patch via DBus too?

I think i will not be able to implement copy image to clipboard feature myself even after reading xclip source code, i had no idea of X11 or wayland before this. And yes there is a danger of making multiple connections (for non-gui/copy-to-clipboard shortcuts), i have just tested it. I think there must be someone workaround for this, I can look for it but i am not sure about implementing the copy image to clipboard feature for x11 myself.

I tried using xclip for this awhile ago. We can add this feature by using QProcess and starting an external process but xclip needs to be installed for this to work. klipper does not seem to support copying images over dbus.

In D9145#178634, @rkflx wrote:

Latest version LGTM. Thanks a lot again for your patch.

In D9145#178193, @rkflx wrote:

In D9145#178033, @rkflx wrote:

I think I found a way to fix the problem properly.

Here we go, fixes submitted for review in D9281 and D9282.

Backstory how I figured it out:

Looking at the other toolbuttons, I saw they worked fine. Then I added some text to the zoom buttons in the status bar and even wrote a test app with only some buttons in different layouts, all working fine. The only explanation left was that there had to be a bug somewhere, meaning it was not Gwenview's fault. And indeed, switching styles the problem happened only with Breeze and Oxygen. But still all other toolbuttons worked fine, how could that be?

Only by chance I stumbled upon the _kde_toolbutton_alignment property in GammaRay. Tracing this one back was not straightforward, but I ended up in Breeze. Seeing the special casing for Gwenview there was quite a suprise, to say the least. One git blame, some code reading and thorough testing later the patch was ready.

In D9145#178152, @rkflx wrote:

In D9145#178147, @kapillamba4 wrote:

Can we just set left-margin for title to 3 in constructor of sidebargroup, also set container margin to 3 for Image operation, file operation and meta information and set margin 3 inside ui file for semantic information (not whole of the container)

Depending on what you refer to with "container", the title might have double margin now? Just try it, I guess.

If you want to keep the ui file, I would be okay with that. But instead of the margin setter call at least add a comment to that effect at a similar place in the code.

In D9145#178097, @rkflx wrote:

I think the best way would be to try to get rid of semanticinfosidebaritem.ui, i.e. manually create its items in code and essentially have in each sidebar a container (red rectangle) which is then indented at a central place:

In D9117#177492, @ngraham wrote:

Does klipper support non-text clipboard data? If so, you might consider storing it there rather than in the X11 clipboard.

In D9145#178033, @rkflx wrote:

Can you remove this? I think I found a way to fix the problem properly. Still needs some fiddling, but I'll have it figured out soon™ (sorry for the cliffhanger :)

In D9145#177362, @rkflx wrote:

Ok, then we agree. At least we tried and learned something in the process…

yup

Updated:

I actually don't think text input widgets look bad with or without margin.

In D9145#177316, @rkflx wrote:

I'm so sorry I have to tell you this, but now it breaks in another place. I'm confident you'll get it right eventually :)

Also, I guess you are still working on the margins of the text input widgets as per my comments?

---

Edit: Can't edit the inline comment, of course I mean "do NOT remove"…

For QT_SCALE_FACTOR=1.4 :

I don't think fractional scaling is supported by qt:
https://www.mail-archive.com/kde-devel@kde.org/msg09355.html
https://bugzilla.redhat.com/show_bug.cgi?id=1381828#c1
https://bugreports.qt.io/browse/QTBUG-55654

In D9145#176861, @rkflx wrote:

Thanks, I'll look at your updates in a bit.

Meanwhile, please let us know which email address should be used when landing this patch on your behalf.

In D9145#176787, @ngraham wrote:

Lovely, much better now!

yup, 👍

I still don't know why normal conventional methods like setting stylesheet or setContentsMargin are not working for QToolButton

In D9145#176147, @rkflx wrote:

In D9145#176143, @kapillamba4 wrote:

does this look good?

The space between icon and text now looks much narrower than without the patch. I think it was just fine in an unpatched Gwenview, it is only the left margin which needs tweaking. Note that in my screenshot I set the toolbar icon size to "small" to match the sidebar buttons. You can use pixeltool-qt5 or gammaray to make measuring easier.

---

As for the summary, please don't just repeat the title ;) Write what was wrong (i.e. section header text as well as buttons too close to edge of window when setting window borders to "none") and what fixes this.

@ngraham , @rkflx does this look good?

I only see margin "3" at 4 places, 3 of which will be added by this patch and
most of the "margin" values seem to be zero.

Seems to work with HiDPI for me as well:

Copy to Clipboard does not persist after app closes
Just found about this bug