Add docbook content which was lost during a merge and more cleanup
ClosedPublic

Authored by rkflx on Feb 15 2018, 1:57 PM.

Details

Summary

45c37a64e1cc lost some docbook changes from 7034b50c5c9e, let's restore
them.

Also bump date and releaseinfo, and remove entities which now come from
kdoctools.

Thanks to @gregormi for the initial version of the patch, and @lueck for
help with the docbook.

Depends on D10529

Test Plan

checkXML5 index.docbook
Proofread in KHelpCenter.

Diff Detail

Repository
R166 Spectacle
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
gregormi requested review of this revision.Feb 15 2018, 1:57 PM
gregormi created this revision.
gregormi retitled this revision from Summary: Add docbook content which was lost during merge to Summary:Add docbook content which was lost during merge.Feb 15 2018, 1:58 PM
gregormi added a reviewer: Spectacle.
gregormi retitled this revision from Summary:Add docbook content which was lost during merge to Add docbook content which was lost during a merge.
gregormi edited the summary of this revision. (Show Details)
rkflx added a subscriber: rkflx.Feb 15 2018, 2:14 PM

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.

rkflx added a comment.EditedFeb 15 2018, 2:24 PM

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 ContentsCommits 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 ;)

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.

rkflx added a subscriber: ngraham.Feb 15 2018, 2:34 PM

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…

rkflx added a comment.EditedFeb 15 2018, 2:50 PM

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!

rkflx added a comment.Feb 24 2018, 6:21 PM

@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.

@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.

rkflx added a comment.Mar 6 2018, 8:38 PM

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

@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.

lueck added a subscriber: lueck.Mar 7 2018, 11:30 AM

Please add group Documentation as reviewer for docbook changes

doc/index.docbook
6–7

please remove these entities from the header
since 2016-09-17 these entities are already defined in kdoctools:
https://cgit.kde.org/kdoctools.git/commit/src/customization/entities/contributor.entities?id=e7389ef900e8e606aaba3074de3bfe52172c8705

34–35

please bump date + releaseinfo, this docbook is for Spectacle 18.04

rkflx commandeered this revision.Mar 8 2018, 10:29 PM
rkflx added a reviewer: gregormi.

Taking over from @gregormi as agreed above (hope your flu is getting better now ;)

rkflx updated this revision to Diff 29048.EditedMar 8 2018, 10:47 PM
rkflx retitled this revision from Add docbook content which was lost during a merge to Add docbook content which was lost during a merge and more cleanup.
rkflx edited the summary of this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)
  • Rebase on tabs Diff to separate whitespace changes
  • Reorder to match Tools menu
  • Minor editing
  • Burkhard's suggestions
rkflx marked 2 inline comments as done.Mar 8 2018, 10:51 PM

@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.

Restricted Application added a subscriber: kde-doc-english. · View Herald TranscriptMar 13 2018, 12:46 AM
lueck accepted this revision.Mar 13 2018, 3:34 AM
This revision is now accepted and ready to land.Mar 13 2018, 3:34 AM
This revision was automatically updated to reflect the committed changes.