Thanks for the fix.

Seems like you based this on D10529 instead of master, but let's not change tabs in this patch. Also, there are some line breaks you are accidentally introducing (check with F10 in Kate).

Next, the order of the options should match between documentation and application, which it does not at the moment. We need to figure out what we want. Personally, I'd use Open Screenshots Folder the most, but that's just me and I also implemented it :D. Perhaps Record Screen at the bottom, because it doesn't actually operate on the screenshot at all (no offense :)? Anyway, I'm open to suggestions to find something usable and logical.

In D10542#207008, @rkflx wrote:

Seems like you based this on D10529 instead of master, but let's not change tabs in this patch.

To elaborate: See Phab's docs. By default, arc diff will take all commits against master for your new Diff. Looking at Revision Contents → Commits this seems to be what's going on here. You could fix this via one of these alternatives:

• git checkout master; arc feature docbook-fix; git cherry-pick a5d3e86b65c6; arc diff
• arc diff HEAD^ (but watch out when landing D10529, if you do it this way)

Just check with arc which ;)

In D10542#207008, @rkflx wrote:

Thanks for the fix.

Seems like you based this on D10529 instead of master, but let's not change tabs in this patch.

I forgot to say, that we have to wait until the Tabs/Spaces issues is cleared. I will then adapt the change accordingly

Also, there are some line breaks you are accidentally introducing (check with F10 in Kate).

I put them in deliberately :-). So, is it correct that manual line breaks should not be done in docbook files?

Next, the order of the options should match between documentation and application, which it does not at the moment. We need to figure out what we want.

+1

Personally, I'd use Open Screenshots Folder the most, but that's just me and I also implemented it :D.

+1. By the way: I tried it out and it only highlighted the Pictures folder in Dolphin. From the title "Open Screenshots Folder", I would expect that the folder is actually opened. And ideally the screenshot file is selected.

Perhaps Record Screen at the bottom, because it doesn't actually operate on the screenshot at all (no offense :)? Anyway, I'm open to suggestions to find something usable and logical.

Non taken :). I also think, Record Screen should go last. The order in my last patch resulted from the merge. I just kept it that way.

In D10542#207044, @gregormi wrote:

I forgot to say, that we have to wait until the Tabs/Spaces issues is cleared. I will then adapt the change accordingly

You could add a dependent revision, but I'd prefer to keep both fixes separate TBH (not sure how long the other Diff will take to get reviewed).

Also, there are some line breaks you are accidentally introducing (check with F10 in Kate).

I put them in deliberately :-). So, is it correct that manual line breaks should not be done in docbook files?

I did not spot them anywhere else and I'm not sure how they work with the translations. Perhaps better to not put them in.

Next, the order of the options should match between documentation and application, which it does not at the moment. We need to figure out what we want.

+1

Okay, great. Unless @ngraham disagrees, you'll adapt the docbook accordingly here, and I'll do the rest tonight.

By the way: I tried it out and it only highlighted the Pictures folder in Dolphin. From the title "Open Screenshots Folder", I would expect that the folder is actually opened. And ideally the screenshot file is selected.

I tried to handle that, not sure why it fails for you. Could you provide exact steps to reproduce, i.e. state of Save button, did you take a screenshot via the button etc.? And perhaps take the discussion over to D10470, if you don't mind…

In D10542#207055, @rkflx wrote:

In D10542#207044, @gregormi wrote:

By the way: I tried it out and it only highlighted the Pictures folder in Dolphin. From the title "Open Screenshots Folder", I would expect that the folder is actually opened. And ideally the screenshot file is selected.

I tried to handle that, not sure why it fails for you. Could you provide exact steps to reproduce, i.e. state of Save button, did you take a screenshot via the button etc.? And perhaps take the discussion over to D10470, if you don't mind…

Okay, don't bother – I found the problem, which was pre-existing and not really a problem with my patch. It happens only if you never used Save As but Save As is selected in the Save button. Spectacle writes Pictures/ to the config file, but if this is empty it returns Pictures, so we get exactly what you observe. I'll fix it shortly.

And ideally the screenshot file is selected.

If the screenshot is Unsaved*, obviously we cannot highlight it…

No objections!

@gregormi Ping.

Take your time with the spaces/tabs fix in D10529, but this Diff is a fix for a merge which did go wrong, so it should be landed sooner rather than later.

In D10542#212839, @rkflx wrote:

@gregormi Ping.

Take your time with the spaces/tabs fix in D10529, but this Diff is a fix for a merge which did go wrong, so it should be landed sooner rather than later.

Hi, sorry for leaving this open for so long. I actually have lost track what should be done. I have to acquaint myself again in this Diff which I can do by the end of next week. If you want to take over, please give me a short note in case I find the time earlier.

@gregormi Sorry to bother again. If you are busy currently, just tell me and I'll take over.

In D10542#220252, @rkflx wrote:

@gregormi Sorry to bother again. If you are busy currently, just tell me and I'll take over.

It is ok to bother because my promised response was due last weekend :). Last weekend I also got the flu, so am I am grateful if you could take over.

Please add group Documentation as reviewer for docbook changes

@lueck Ready for Documentation review… (You might also want to look at more recent changes we did to Spectacle's docbook since the beginning of the year, where we forgot to tag the docs team.)

(…actually add the correct reviewer…)

@lueck Please let me know until tomorrow if I should change anything, or accept this review on Phab otherwise.