Refactor Spectacle's platform backends
Needs ReviewPublic

Authored by bgupta on May 5 2019, 10:33 PM.

Details

Reviewers
ngraham
davidre
Group Reviewers
Spectacle
Summary

This patch refactors Spectacle's platform backends.

The platform backends (esp. Xcb) is now more stateless. Additionally, the rectangular cropper is now taken out of the platform backends and is directly invoked by the core. This has the happy side effect of making it work on Wayland. The experience might be suboptimal on multi-screen setups, but it's better than nothing. The on-click mode is also now correctly locked out and force-enabled on KWinWayland, since KWin enforces making the user click when a screenshot is taken.

There's also some cleanups and coding style changes:

  • New license header with a SPDX-License-Identifier (only files that I touched)
  • New way to consistently name variables (locals prefixed with l, members with m and function arguments with the, and optional arguments suffixed with Opt and output reference arguments suffixed with Out). This is from the coding style guide of a certain large European enterprise software vendor (the entire style guide is internal and I can't make the whole thing public) and I find this makes navigating new codebases really easy because you immediately know the scope of a variable when you see the name.
  • Bump C++ language requirements to C++14 and also bump CMake requirements to CMake 3.10.
  • Use more stdlib than Qt stuff whenever possible, esp. smart pointers.

Diff Detail

Repository
R166 Spectacle
Lint
Lint Skipped
Unit
Unit Tests Skipped
bgupta created this revision.May 5 2019, 10:33 PM
Restricted Application added a subscriber: Spectacle. · View Herald TranscriptMay 5 2019, 10:33 PM
bgupta requested review of this revision.May 5 2019, 10:33 PM
bgupta edited the summary of this revision. (Show Details)

I don't know how Phab works with submitting patches directly from the web interface or through arc, but please don't merge this - the way to submit this change is to merge the bgupta/platform-backend-refactor branch to master.

nicolasfella added inline comments.
CMakeLists.txt
26

The comment says 17 but the code 14

bgupta marked an inline comment as done.May 6 2019, 12:10 AM
bgupta added inline comments.
CMakeLists.txt
26

Good catch, I'll fix the comment before pushing.

bgupta edited the summary of this revision. (Show Details)May 6 2019, 12:45 AM
ngraham requested changes to this revision.May 6 2019, 3:00 AM
ngraham added a reviewer: Spectacle.

In general +1, very nice. I haven't tested the now-working Wayland Rectangular Region mode yes, but I will.

I have to say I don't really like how the proposed new the prefixing style works for boolean variables. It leads to awkwardness like if (theNotifyOnGrab) {. Making the name a noun makes me think it's some object, not a boolean. Compare that to the status quo: if (notifyOnGrab) {, which I think is more readable.

Also prepending l to local variables makes conceptual sense but I admit it looks a bit odd to me given that's not used anywhere else in the KDE style guide. It may be more consistent for you to code Spectacle the way you would at work, but it's less consistent for everyone else in the KDE community who doesn't work there. Ultimately you're the maintainer but KDE has its own set of coding style guidelines and in general I think it might be best to stick with it, or at least not deviate from it too much. :) I won't make hay about it though.

However I found one block-ship regression in testing: after dragging the Rectangular Region box with "Accept on click-and-release" mode active, Spectacle reproducibly crashes.

CMakeLists.txt
10

That's pretty darn new. A lot of distros ship an older version and their users won't be able to compile Spectacle.

src/Platforms/PlatformXcb.cpp
643

JFYI in the future it would be nice to be able to make this (and including the window decoration shadows) optional. See https://bugs.kde.org/show_bug.cgi?id=372408

This revision now requires changes to proceed.May 6 2019, 3:00 AM
davidre added a subscriber: davidre.May 6 2019, 6:34 AM

Have to agree with Nate regarding the naming convention you are using - feels odd to me. I don't think lConfigMgr is more readable than configManager. Also following a style guide which you can't make public is a strange move in a community like KDE.

There are some whitespace changes in the code and together with renaming everything they make looking for the relevant changed hard. This is a large diff! Made even larger by the whitespace changes in the license headers.

Also -1 to #pragma once . It's not standard and it has the same functionality as include guards.

src/Gui/KSWidget.h
72–88

Why the explicit initializations to nullptr?

davidre requested changes to this revision.May 6 2019, 6:34 AM
davidre added inline comments.May 6 2019, 7:52 AM
src/SpectacleCore.h
88

These don't need to be unique_ptr

bgupta marked an inline comment as done.May 6 2019, 9:01 AM

In general +1, very nice. I haven't tested the now-working Wayland Rectangular Region mode yes, but I will.

I have to say I don't really like how the proposed new the prefixing style works for boolean variables. It leads to awkwardness like if (theNotifyOnGrab) {. Making the name a noun makes me think it's some object, not a boolean. Compare that to the status quo: if (notifyOnGrab) {, which I think is more readable.

Also prepending l to local variables makes conceptual sense but I admit it looks a bit odd to me given that's not used anywhere else in the KDE style guide. It may be more consistent for you to code Spectacle the way you would at work, but it's less consistent for everyone else in the KDE community who doesn't work there. Ultimately you're the maintainer but KDE has its own set of coding style guidelines and in general I think it might be best to stick with it, or at least not deviate from it too much. :) I won't make hay about it though.

This was jarring for me when I first started looking at code written with this convention, but I got used to it quickly and now I have a hard time looking at code written another way. There's a first time for everything :-) I'd say, let it grow on you.

I will add proper documentation about the variable naming scheme. There's also other KDE contributors at my office and I'm sure someone somewhere uses this convention in their KDE apps.

However I found one block-ship regression in testing: after dragging the Rectangular Region box with "Accept on click-and-release" mode active, Spectacle reproducibly crashes.

Thanks! This is why I scream for mandatory code reviews. I'll take a look today evening.

bgupta added a comment.May 6 2019, 9:14 AM

Have to agree with Nate regarding the naming convention you are using - feels odd to me. I don't think lConfigMgr is more readable than configManager. Also following a style guide which you can't make public is a strange move in a community like KDE.

Apart from my previous comment about "you'll get used to it and then you'll love it" - I'm not following the entire style guide, just the variable naming scheme - which is the only bit in the guide that makes sense to use in other projects - and I've just stated what the naming scheme is.

Also -1 to #pragma once . It's not standard and it has the same functionality as include guards.

I didn't just change to #pragma once out of the blue. This is not part of the standard but is supported by every single compiler that we want to support, is shorter and more expressive and in some cases might be faster (although that's not a reason by itself to use it). The real reason why #pragma once is preferred over ifdef guards is that ifdef guards allow you do hack around and include the same header twice with different #defines before - which means that the header could expand to different content under different circumstances. Some people might consider this an advantage, but in reality (a) you don't need this flexibility and (b) if you do, you're Doing It Wrong (TM) and you should refactor your code - see (a).

You don't have to be scared about #pragma once being nonstandard - enough people rely on its behaviour that if it were to change, a lot of code would break - so this won't go away.

bgupta added inline comments.May 6 2019, 9:22 AM
CMakeLists.txt
10

CMake <= 3.10 can't enforce a C++ standard on older versions of vc++. Unfortunately I can't conditionally set the cmake minimum version. Lemme see what else I can do.

FWIW I use a very old distro (openSUSE 42.3) and I have CMake 3.14 courtesy of obs://build.opensuse.org/windows:mingw - there's always _ways_ of getting it. What I wouldn't like is if official packagers can't package this.

src/SpectacleCore.h
88

What else, shared_ptr? They're scoped to this object.

davidre added inline comments.May 6 2019, 9:30 AM
src/SpectacleCore.h
88

Before they were raw pointers that were deleted in manually in the destructor. Sorry I missed that you defaulted it.

bgupta updated this revision to Diff 57657.May 6 2019, 4:17 PM

Fix crash in release-to-capture, address minor comments

davidre resigned from this revision.May 6 2019, 5:30 PM
ngraham resigned from this revision.May 6 2019, 5:41 PM

Thanks, the crash is fixed now.

I'm still not really on board with some of the style changes though. I understand how this makes hacking on the project easier for you, but it makes it harder for everyone else in the KDE community who isn't you or doesn't work at your company and doesn't follow the same style guide 8 hours a day. I'd be a hard "No" if anyone else proposed this, but you're the author and maintainer, so ultimately it's your call. However I'll note that you disappeared for a few years and left Spectacle to be community-maintained. During this time, I fended off an attempt to incubate yet another screenshot app (KSnip), which would have left Spectacle to die. Spectacle benefited from the fact that the coding style was more coupled to the general KDE style, so it was easier for people to quickly get acquainted with the code and contribute. As a result, Spectacle didn't die during this challenging time for it. I'm happy you're back now, but what happens if and when you leave again? We want for KDE apps to be as accessible as possible to all contributors, not just you. This refactor is generally very welcome, but it also has other changes within it that I think have the potential to the codebase less accessible to other members of the KDE community as well as casual contributors. There's always a balance to be struck here, and I can see that there's not an easy answer.

It's your call, but I encourage you to consider the implications for Spectacle as KDE project, not just as a Boudhayan Gupta project. :)

bgupta added a subscriber: ngraham.May 6 2019, 8:29 PM

Thanks, the crash is fixed now.

I'm still not really on board with some of the style changes though. I understand how this makes hacking on the project easier for you, but it makes it harder for everyone else in the KDE community who isn't you or doesn't work at your company and doesn't follow the same style guide 8 hours a day. I'd be a hard "No" if anyone else proposed this, but you're the author and maintainer, so ultimately it's your call. However I'll note that you disappeared for a few years and left Spectacle to be community-maintained. During this time, I fended off an attempt to incubate yet another screenshot app (KSnip), which would have left Spectacle to die. Spectacle benefited from the fact that the coding style was more coupled to the general KDE style, so it was easier for people to quickly get acquainted with the code and contribute. As a result, Spectacle didn't die during this challenging time for it. I'm happy you're back now, but what happens if and when you leave again? We want for KDE apps to be as accessible as possible to all contributors, not just you. This refactor is generally very welcome, but it also has other changes within it that I think have the potential to the codebase less accessible to other members of the KDE community as well as casual contributors. There's always a balance to be struck here, and I can see that there's not an easy answer.

It's your call, but I encourage you to consider the implications for Spectacle as KDE project, not just as a Boudhayan Gupta project. :)

The whole point of naming variables this way is so that people who are not familiar with the codebase have an easier time navigating it. I'd argue that this actually makes it easier, not harder, for contributors to hop on, including contributors outside KDE and contributors who haven't had any prior experience with this codebase.

I'm going to give this a go. If this seriously puts off people from contributing, I'll think about reverting this, but given how I felt in the initial days of using this style and the way I feel about it now, I wouldn't be surprised if more projects actually adopt this after they've gotten over the initial shock of this style (yeah, shock is how I would describe my first few days with this style :-D)

All right, go ahead. :)