[RFC] Fix/remove command-line options; --delay needs review
Needs ReviewPublic

Authored by sharvey on Feb 27 2018, 1:21 AM.

Details

Reviewers
ngraham
rkflx
Group Reviewers
Spectacle
Summary

Fixed and reworked several command-line options to get them working
properly. "Delay" option needed a 2-second thread sleep in order to
get the notification to appear reliably. Need others to please try
with and without this pause, in case it's unique to me. Removed
"wait for mouse click", as it wasn't implemented. We should consider
removing "window under cursor" because it generates a poor result;
example screenshots will be included.

Diff Detail

Repository
R166 Spectacle
Branch
parserflags (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
sharvey requested review of this revision.Feb 27 2018, 1:21 AM
sharvey created this revision.

In addition to the changes I described in the submission, I propose removing the "grab window under mouse cursor" option, as it doesn't/can't work as one might expect. It grabs the screen geometry bounded by the window under the cursor, but it includes any windows that are overlapping it. Although KWin has an API call to raise a window to the top (which we could then grab), the docs forbid apps from using it.

Please check out the unappealing hack I had to put in to get the notification working when using "delay" mode. I had to add a 2-second thread sleep in order to get the notification dependably. It may be unique to me; I need some testing.

I removed several parser options that either didn't work or weren't implemented. I changed the delay input value to seconds instead of milliseconds. Humans think in seconds.

I'm going to look at a couple of the other open Spectacle bugs/feature requests, while I've got it dissected and fresh in my mind. I know some work was done on copying to the clipboard instead of a file. There are some GUI fixes I think I can clear up. But first I need to delete the dozens and dozens of screenshots I have built up from testing...

@sharvey Thanks for your work. Did not look into every detail yet, but note that the whole topic of what we should support, what works, what needs to be fixed and how to improve the presentation is under discussion at the moment. Apart from the conceptual work to be done, the notification implementation has severe issues as well. And don't forget about the Wayland situation…

T8033
D9117
https://bugs.kde.org/show_bug.cgi?id=389694
https://bugs.kde.org/show_bug.cgi?id=390415

Before I can comment here I'd like to sort through the mess we currently have, which might take a while. I hope you understand I'm hesitant to add more hacks right now or rush any fixes. Your patch containing so many comments in that regard is another indicator.

Can we draw up a big table on Spectacle's workboard with every feature we currently support and how it is accessed (GUI, shortcut, CLI, DBus)? That would be helpful.

Also, when changing command-line options we should think about backwards-compatibility. If we ever decide to make big incompatible changes, those should be done only once, i.e. don't break user's workflows for 18.04 and annoy them for 18.08 again.

Finally we are busy fixing bugs and want to fix even more in the future, so removing non-working options should perhaps be held off too.

@rkflx Thanks for your review. I was taught long ago to comment my code. The parser options were a windy mess of if's and then's and switch'es, so I tried to document what did what and why and in what order. If leaving code comments behind is not the usual routine, I won't do it in the future.

As I told @ngraham (who has graciously agreed to mentor me), I didn't even know Spectacle had command-line options before I began fixing them.

I'll keep on participating in the discussion and hope to remain helpful going forward. At least I've gained familiarity with one product's code.

rkflx added a comment.EditedFeb 28 2018, 8:51 PM

@sharvey We would be grateful if you helped cleaning the command line options later. It's only that we first have to figure out how those should even be designed.

@rkflx Will do. Please consider me an available resource for any junior jobs you would like addressed. I'm happy to do the work, but also realize the missteps I may have made in this particular submission. Thanks for your patience and the opportunity.

rkflx added a comment.EditedFeb 28 2018, 9:40 PM

@rkflx Will do. Please consider me an available resource for any junior jobs you would like addressed. I'm happy to do the work, but also realize the missteps I may have made in this particular submission. Thanks for your patience and the opportunity.

I would not call it missteps, it was just an unfortunate point in time for the changes you had in mind (which go in the right direction!).

As for junior jobs (hopefully, not sure about everyone ;), how about those:

There are also junior jobs for Gwenview, just ask ;)


Before I can comment here I'd like to sort through the mess we currently have, which might take a while.

Just to clarify: With "mess" I did not mean your patch. This was about us not knowing how Spectacle is actually supposed to work ;)

@rkflx I didn't take any offense. I simply wasn't aware of the bigger picture regarding Spectacle's overall design. I'm just getting started with C++, although I'm nearly 50. I know other languages, but this is a challenge.

I'll work on the following, but if you have a moment to clarify...

  • Should CTRL + T be an ordinary shortcut or a DBUS trigger?
  • Please clarify which compiler warnings you'd like cleaned up. The only "warning" I get is that I'm missing the new "Purpose" package. Otherwise, the 'cmake' output is very lengthy, but all successes, at least for me.
  • I'll look at the other open bugs you listed.

Okay, great. I'd suggest to pick a single item and not work on all of them at once ;)

  • Should CTRL + T be an ordinary shortcut or a DBUS trigger?

Sorry, I should have been more clear. I only meant the GUI itself for now. If you have Spectacle open, Take a new Screenshot is defined as the "default" button, meaning that if you press Enter, it will get triggered. My idea was simply to add an additional shortcut reachable with the other hand.

As for a DBus trigger, we have https://bugs.kde.org/show_bug.cgi?id=374770, but that one might be more complicated (although nice to have!). We'd need to figure out first whether Print should start multiple instances or take a new screenshot in an already open app.

  • Please clarify which compiler warnings you'd like cleaned up. The only "warning" I get is that I'm missing the new "Purpose" package. Otherwise, the 'cmake' output is very lengthy, but all successes, at least for me.

CMake relates to the build system, but I was referring to the step afterwards, i.e. when you type make. See our CI platform for an example of the warnings.

Let me know your compiler and how you are building Spectacle if you still cannot reproduce the warnings.

I've been building Spectacle the same way the README's have told me to build other CMake things, not just KDE projects.

  • create a 'build' directory at the project root level and cd into it - mkdir build && cd build
  • issue cmake .. followed by make and make install

I have an alias set up for cmake that uses the -DCMAKE_INSTALL_PREFIX flag (thanks to the devs on IRC for that trick) so I can "install" the finished product to a local directory without needing sudo or overwriting my working install.

My compiler reports:

$ make --v
GNU Make 4.1
Built for x86_64-pc-linux-gnu

... which is what came pre-loaded with KDE Neon. If I should try a different compiler, let me know which one. Or if there's a different technique I should use, I'll try that instead.

I'm off on vacation next week (Denmark! Sweden!), so you won't hear from me after tomorrow.

I promise to tackle these one at a time. I'm erring on the side of caution.

Follow-up, now that I've read up on the differences between make and one's actual compiler (sorry, learning), my compiler is gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.9). I found that Jenkins is using the most current version, 7.3. That seems appropriate for a tool like Jenkins. I'll explore setting up gcc 7.3 for myself and see if I get the same compiler warnings.

rkflx added a comment.Mar 1 2018, 12:13 AM

Take your time, and have a nice vacation. We are busy with other things anyway.

Was just drafting a comment regarding your issue, but I think you've found the culprit ;)

rkflx resigned from this revision.Aug 25 2018, 6:42 AM