Now that my compilers are updated to v7.3.0, I saw the same warnings that Jenkins reported. I believe I have fixed them appropriately.

@ngraham As a reviewer, please indicate what testing you've done.

Compiled with the patch, took screenshots in every mode using the GUI, ran spectacle --background. Everything worked as expected.

showErrorMessage(i18n("Screenshot capture canceled or failed"));

@ngraham @sharvey The code touches this part. Did you try to provoke this message and see whether everything worked as before? Similar concern for the other hunk above.

(I did not test yet, but looking at the code we see that before two signals were emitted, while now they are not. Are those signals important?)

I tried to provoke the error message by setting a delay and Ctrl-C'ing the paused terminal. I couldn't trigger the error this way and couldn't come up with a better test. If you have any ideas on how to test it, I'll try them out.

Will do once I had time to look at the code (to see where those functions are called from, and perhaps rewire them manually).

Another strategy would be to follow through the code base how the code is executed by hand.

Basically we need to make sure whether the "fallthrough" the compiler warned us about was intentional or not. In my book the commit message ("Summary") and/or test plan should contain something about this topic.

BTW, I assume we are talking about the same thing?: https://en.wikipedia.org/wiki/Switch_statement#Fallthrough

I was working with the assumption that the break/fallthrough were intended as that definition describes. I may be mistaken, but my reading of the code didn't have any default code below the end of the switch cases. I'm still learning my C++, but that appeared to be the intent as I've read & learned the "proper" use of a switch statement. The matched expression's code had been run through and nothing remained, so issue a "break" to end the case block.

I will work on writing more detailed commit & testing messages in the future.

Writing an example program is quite useful for trying out how switch works and what default does: http://cpp.sh/5j764

In the example shown at that link, it's my belief that case 0: should end with a break; statement to prevent the fallthrough into case 1:. The default: block runs as expected when 99 is passed to the function, as it doesn't match any of the defined cases.

In D10936#216510, @sharvey wrote:

In the example shown at that link, it's my belief that case 0: should end with a break; statement to prevent the fallthrough into case 1:.

That's exactly what the compiler warns about: break might be missing. We don't know for sure, though, perhaps the programmer wanted exactly the output as defined in that program. In general this is to be avoided because it can result in confusion and subtle mistakes, so we get the warning, but it's not forbidden or an error.

That's what you have to check for Spectacle. If you determine that both emits are important also in the first case, you'd have to repeat them. (There are also ways to only silence the warning, but let's not use those.) If they are unimportant, say why that is in the commit message.

In my mind, letting code execute by fallthrough into the next case seems incorrect, even if it's not technically an error state. I wouldn't code something that way. As you say, it's confusing to the next person who comes along and tries to resolve a compiler warning. ;-)

The way I understand the purpoe of switch and case is to avoid the need for a complicated block of if/else statements. The expected cases are defined and handled individually (i.e. without fallthrough); anything not matching a case is to be handled by the default block. Any code to be run under all circumstances should be outside the switch definition to avoid repetition.

I think you understood what switch is about, and what best practices for using it are. Now look at Spectacle and try to find out whether grabFailed and allDone are needed for BackgroundMode or not.

I'm new to this, so bear with me.

The only place I find grabFailed() being handled is in Main.cpp at line 150. It appears to be unique to DBus mode. Everything else is handled by imageGrabFailed(), which is again confusing.

As for allDone(), it appears to simply quit the application:

QObject::connect(&core, &SpectacleCore::allDone, qApp, &QApplication::quit);

(Main.cpp, line 143)

So my hunch is that emitting allDone() in background mode would be appropriate. The case calls showErrorMessage which prints an error to the console by way of qDebug() in background mode. If we don't have the return; statement, the case falls through onto the DBus error case, which in turn emits allDone and shuts down the application.

Perhaps my submission should be revised to emit allDone rather than return;.

In addition, I believe I found a way we might trigger this for testing purposes, but I'm unsure. In ImageGrabber.cpp, we could send an invalid mGrabMode, which, as luck would have it, uses a switch and default block to trigger an error:

void ImageGrabber::doImageGrab()
{
  switch(mGrabMode) {
  case FullScreen:
      return grabFullScreen();
  case CurrentScreen:
      return grabCurrentScreen();
  case ActiveWindow:
      return grabActiveWindow();
  case WindowUnderCursor:
      return grabWindowUnderCursor();
  case TransientWithParent:
      return grabTransientWithParent();
  case RectangularRegion:
      return grabRectangularRegion();
  case InvalidChoice:
  default:
      emit imageGrabFailed();  <--------
      return;
  }
}

Doing this would require changing the code so a capture mode sets mGrabMode to an invalid value. I'm unclear if this constitutes a valid test or not.

Thank you for the tutoring. I hope we can wrap this up before my time expires and I'm forced to go on vacation.

Thanks for your investigation, I believe you learned something ;)

In D10936#216591, @sharvey wrote:

Thank you for the tutoring. I hope we can wrap this up before my time expires and I'm forced to go on vacation.

I'd have to look at the code myself, but currently I'm busy elsewhere – sorry! We can come back to it later.

I’m curious to figure out how I broke your product. :-)

I’ll try doing that on my own before calling for extra help. I know you’re busy and have already been quite patient with me. I’ll work through the various choices you outlined after I get home this weekend.

In D10936#222187, @sharvey wrote:

I’m curious to figure out how I broke your product. :-)

It's our (KDE's) product, i.e. yours too ;)

I'm filling up my Pictures folder with screenshots, trying to find the (in)correct combination of command-line options to break Spectacle. The best I've been able to generate is a minor error:

./spectacle -b -r
Xlib:  extension "NV-GLX" missing on display ":0".

But even this successfully grabbed the screen. I don't think this is what you're referring to.
Can I ask for either:

• a pointer on how to figure it out, or
• the cursed combination of options

I think a pointer on how to find the problem is the best request. Since I must not have known what behavior I inadvertently broke, I'm having a hard time making it break. Spectacle is full of features and options; I'm still discovering them as we dig deeper and deeper into this.

Okay, let's do it this way: I'll tell you some things, and leave the rest for you to investigate.

First off, let's talk about SpectacleCore::screenshotFailed:

In D10936#216473, @rkflx wrote:

showErrorMessage(i18n("Screenshot capture canceled or failed"));

The code touches this part. Did you try to provoke this message and see whether everything worked as before?

Following the code backwards, it goes something like this:

screenshotFailed ← imageGrabFailed ← rectangleSelectionCancelled ← grabCancelled ← cancelImage ← onEscapePressed

This means by running spectacle -b -r and then pressing Esc, you can get to this error message. This appears as before, so that's not the problem.

However, we can observe the effect of adding the return and thus making grabFailed unreachable in BackgroundMode: In another terminal, dbus-monitor will not show anymore the ScreenshotFailed signal being emitted (use Ctrl+⇧+F to find it in Konsole).

That is clearly a change in behaviour. I guess that's to be expected and sensible when running in background mode instead of in DBus mode, so that's probably okay if you change that. Still, I'm not sure you were aware of that, even though such things are important to analyze. What you should do here is add a note to the commit message regarding that.

Next, we see in the code that allDone is not reached anymore. You correctly concluded that this will quit the application. Surprisingly, even with your patch Spectacle quits and then returns to the shell prompt, but why? This happens only because at the same time you are also adding the break, so that setQuitOnLastWindowClosed remains true and Qt quits Spectacle without us needing to explicitly emit allDone.

In a way you were lucky up until now nothing broke because the changes cancelled each other, but (spoiler) in the end you'll have to add back allDone so other things won't break. Apart from that, there is another reason keeping this might be good: Maybe in the future someone will connect more functionality to that signal which should also be executed in BackgroundMode.

Now for your change to Main.cpp: It might be a bit tricky to observe, because it depends on timing. It may very well be that on your machine you won't see it. In that case just let me know. What I'm hinting at is this: Run spectacle -b w/ and w/o your patch and watch whether Plasma notifications are shown or not.

As for why this effect is observable, I'm not sure. All I can see is that with your patch it stops working, so the way it was coded before had probably its reasons.

If these explanations help, please update your patch accordingly. If not, that's not a problem at all. I'll just show you what I want, so we are not endlessly working on this…

I discovered the -b -r combination spontaneously this afternoon, trying to take my own rectangular region screenshot. I hadn't gotten back here to report that, since that was all I had to report.

I will work on the remainder of the project with your comments at hand. Thanks for the guidance. Parts of the system are still a black box to me; the D-Bus mechanism is one of them. I'll do some studying.

Good luck with the 18.04 beta. I can tell it's a hectic time for everyone.

Now for your change to Main.cpp: It might be a bit tricky to observe, because it depends on timing. It may very well be that on your machine you won't see it. In that case just let me know. What I'm hinting at is this: Run spectacle -b w/ and w/o your patch and watch whether Plasma notifications are shown or not.

As for why this effect is observable, I'm not sure. All I can see is that with your patch it stops working, so the way it was coded before had probably its reasons.

I see Plasma notifications under both conditions - with the break statement in place, and without. I've stepped through the execution using the Qt Creator debugger. The only possibly significant change I can see is that the execution does fall through, which in turn executes the app.setQuitOnLastWindowClosed(false); statement, inside the case block for DBusMode.

Maybe it is a quirk of timing, as you alluded to. The upshot is that I'm not seeing what you're seeing. If you'd like me to resubmit the patch without the break statement, that's fine. Or if you have a solution, at this point, you can just let me know and I'll paste it in.

If these explanations help, please update your patch accordingly. If not, that's not a problem at all. I'll just show you what I want, so we are not endlessly working on this…

I've added emit allDone() to the failed-screenshot portion of SpectacleCore.cpp. I think I understood your meaning and the plan for that portion.

I don't want to keep spending time at half-measures and half-fixes for this, either. I know it was supposed to be a simple request that expanded into a vast, wide-ranging expedition. I'm not giving up or wavig the white flag, but I think we both want this wrapped up. I won't be offended if you won't be offended - you can just tell me what you'd like at this point. I know you're busy with much more substantive things.

Maybe it is a quirk of timing

Yes, there is definitely something quirky in Spectacle. Apparently quitting directly (i.e. with break) is sometimes so fast that the notification cannot be shown properly anymore before shutting down the app. Going the emit route (i.e. without break), that's not the case (not sure why, though).

This change also removes the emission of signal 'grabFailed()' during a BackgroundMode failure, which only happened due to the fallthrough into the DBus block.

"…but signalling the failure on the bus is not needed in background mode." Please add both to the summary on the top.

Don't give up now ;) We are nearly there, then I'll land this on your behalf.

Don't give up now ;) We are nearly there, then I'll land this on your behalf.

My apologies. I had grown a little frustrated at my inability to resolve something that appeared so simple. I think we have it now. Thanks for all your help.

Although I've noticed a new warning has cropped up in QuickEditor.cpp... (cue mysterious music). Maybe for my next trick.