Skip to content

Conversation

@Olf0
Copy link

@Olf0 Olf0 commented Aug 12, 2023

Olf0 added 14 commits August 12, 2023 03:33
… to `backupWorkingPatchList` and `restorePatchList`
Reference: sailfishos-patches#437 (comment)
¿Is the masking of the **"** with **\** correct in C++?
Better, but still not really good, IMO.  Issue is, it must be terse enough to fit in a single line in portrait orientation on a Jolla 1 (i.e., 540 pixels width)!
Sorry, I became carried away: Originally I only adjusted the original lines 307 & 310, then started fixing a few typos, enhanced a few phrases etc. etc. and ended up in going over the whole file.

Open points:
- Line 110: "\uicontrol"
  Is this correct markup?  Does the sentence make sense this way?
- Line 161: "\badcode"
  Is this correct markup?
- Line 225: "In this state, all enabled services are disabled, and PM must be reactivated via the GUI."
  "Patches" instead of "services"?
- Line 289: "\warning"
  Not "Warning"?  I.e., is it a markup term?
Questions:
- Was capitalising "\c Patches" in line 372 O.K.?
- Is the "\sa" in line 373 correct?
Please review thoroughly!  It became obvious, that some copy&paste flaws existed, but I am not sure, if I rectified them correctly.  I simply assumed that the code is correct, but maybe (unlikely) the documentation mismatch was the other way around.
Notes / questions:
- Line 587: "()"?  Function name missing?
- Line 1103: "on the bus" -> "on D-Bus"?
- Line 1198: Ambiguous, especially the second "all".
- Line 1705: Not better "/sa" instead of "See also"?
- Line 2121: Is that an accidentally deleted comment: Check `git blame`
@Olf0
Copy link
Author

Olf0 commented Aug 13, 2023

Notes:

  1. Please first review, and approve or request changes to [patchmanager-tool] Alter option -h to -?, … sailfishos-patches/patchmanager#450
  2. Questions WRT index.qdoc
    • Line 110: "\uicontrol"
      Is this correct markup? Does the sentence make sense this way?
    • Line 161: "\badcode"
      Is this correct markup?
    • Line 225: "In this state, all enabled services are disabled, and PM must be reactivated via the GUI."
      I do not comprehend this sentence. Maybe "Patches" & "inactive" instead of "services" & "disabled", then I would understand it, but is this what it is supposed to mean? Or a different menaning & rectification?
  3. Questions WRT patchmanagerobject.cpp
    • Lines 344 - 387: Please review thoroughly, because some copy&paste flaws in the comments apparently existed, but I am not sure, if I rectified them correctly. I also assumed that the code is correct, but maybe (unlikely) the documentation mismatch was the other way around.
    • Line 587: "()"? Function name missing?
    • Line 1103: "on the bus" -> "on D-Bus"?
    • Line 1198: Ambiguous, especially the second "all".
    • Line 1705: Not better "/sa" instead of "See also"?
    • Line 2121: Is that an accidentally deleted comment? Maybe checking git blame helps to clarify that.

HTH

@Olf0 Olf0 marked this pull request as ready for review August 13, 2023 02:43
Copy link
Owner

@nephros nephros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! See comment below for detailed answer.

@nephros
Copy link
Owner

nephros commented Aug 14, 2023

Thanks a lot for digging through this.

I have another branch here, more-docs which contains some corrections I made meanwhile.
I propose we do the fixup things there.

Would you mind If I split out the corrections NOT related to the method renaming from this tree, and add them manually at more-docs? Edit: scratch that, all the changes are good here.

2. Questions WRT `index.qdoc`
   
   * Line 110: "\uicontrol"
     Is this correct markup?  Does the sentence make sense this way?
   * Line 161: "\badcode"
     Is this correct markup?

I believe so. \badcode is anything qDoc can't markup, i.e. everything that is not C++ or QML.
\uicontrol is supposed to markup something that is, well a button or menu or something. In effect, i just marks the word in bold.

Ref: https://doc.qt.io/qt-6/27-qdoc-commands-alphabetical.html

   * Line 225: "In this state, all enabled services are disabled, and PM must be reactivated via the GUI."
     I do not comprehend this sentence.  Maybe "Patches" & "inactive" instead of "services" & "disabled", then I would understand it, but is this what it is supposed to mean?  Or a different menaning & rectification?

You're right, it must be "patches", although I believe the daemon will also not really start in this mode (it is then started gain by the mysterious "Resolve Failure" pulley menu option).

3. Questions WRT `patchmanagerobject.cpp`
   
   * Lines 344 - 387: Please review thoroughly, because some copy&paste flaws in the comments apparently existed, but I am not sure, if I rectified them correctly.  I also assumed that the code is correct, but maybe (unlikely) the documentation mismatch was the other way around.

That is very possible, documentation is in a best-effort kind of quality state right now.

   * Line 587: "()"?  Function name missing?

Typo, should be deleted.

   * Line 1103: "on the bus" -> "on D-Bus"?

yup.

   * Line 1198: Ambiguous, especially the second "all".

"Returns all versions contained in the metadata of all patches, indexed by patch name."

   * Line 1705: Not better "/sa" instead of "See also"?

The problem with \sa is that it needs valid linking targets, which it often can't find if they're in another page, or sometimes it links the wrong thing.
Not sure why I chose to write it this way in this particular case, I'll have to se if \sa can do the right thing here.

   * Line 2121: Is that an accidentally deleted comment? Maybe checking `git blame` helps to clarify that.

Indeed it does, it's ancient: https://github.com/nephros/patchmanager/blame/b7c911798a87638508c59b4d77d45acbd8ddb238/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L2118

Can be deleted.

HTH

Oh yes it does, thank you.

nephros pushed a commit that referenced this pull request Aug 14, 2023
@nephros
Copy link
Owner

nephros commented Aug 14, 2023

   * Line 1705: Not better "/sa" instead of "See also"?

The problem with \sa is that it needs valid linking targets, which it often can't find if they're in another page, or sometimes it links the wrong thing.
Not sure why I chose to write it this way in this particular case, I'll have to se if \sa can do the right thing here.

To be precise, the \sa command accepts a comma-separated list of words, all of which it tries to make a link of. In this case I want to point to a specific section of another page, which \sa is not the right finction to use for.

I have reworded the section in more-docs.

nephros pushed a commit that referenced this pull request Aug 14, 2023
@nephros
Copy link
Owner

nephros commented Aug 14, 2023

I have another branch here, more-docs which contains some corrections I made meanwhile. I propose we do the fixup things there.

In fact I have started this. I also have pushed that branch to the main repo so you can push to it without doing a PR here:

https://github.com/sailfishos-patches/patchmanager/tree/more-docs

(... AND you can use a manual trigger of the building action to see what the results look like.)

@nephros nephros merged commit 6ab1ddd into nephros:recover-failures Aug 14, 2023
@Olf0 Olf0 deleted the rename-both-new-methods branch August 14, 2023 16:16
@Olf0
Copy link
Author

Olf0 commented Aug 14, 2023

Thanks a lot for digging through this.

You are welcome. Though the documentation enhancements were more or less accidentally, due to becoming carried away when carrying out the name changes.

I have another branch here, more-docs which contains some corrections I made meanwhile. I propose we do the fixup things there.

Would you mind If I split out the corrections NOT related to the method renaming from this tree, and add them manually at more-docs? Edit: scratch that, all the changes are good here.

Oh, I could have. I already considered splitting the name changes and the documentation enhancements, but without being aware of the more-docs branch, there was little incentive to do that, except for clarity of the workflow.

I also have pushed that branch to the main repo so you can push to it without doing a PR here:

https://github.com/sailfishos-patches/patchmanager/tree/more-docs

I do not mind which branch to pose a PR against: It is Git, it does not really matter.

But this …

(... AND you can use a manual trigger of the building action to see what the results look like.)

… allows to perform quality assurance on the output, not only the input, which enables to perceive and address many additional aspects.


P.S.: Nitpick

Ref: https://doc.qt.io/qt-6/27-qdoc-commands-alphabetical.html

Thanks you! With this reference at hand, I looked up as the 100% appropriate reference (for us, here, AFAIU) https://doc.qt.io/qt-5/03-qdoc-commands-markup.html

@Olf0
Copy link
Author

Olf0 commented Aug 14, 2023

… becoming carried away when carrying out the name changes.

I have another branch here, more-docs which contains some corrections I made meanwhile.

Would you mind If I split out the corrections NOT related to the method renaming from this tree, and add them manually at more-docs? Edit: scratch that, all the changes are good here.

@nephros, I wanted to carry on enhancing the documentation, but now it is the time to consolidate the work first, IMO, because I would like to continue based upon the changes in Patchmanager PR #437 (i.e, the nephros:recover-failures branch), but there is a separate, overlapping changeset in the more-docs branches (which are currently on par in your and Patchmanager's repo), I initially was not aware of. Now that I am, I hesitate to contribute to further divergence of various changesets at different locations: Let us please merge it all into master after now that I made a new release.

nephros pushed a commit that referenced this pull request Nov 26, 2024
…List` (#6)

* [org.SfietKonstantin.patchmanager.xml] Rename both new methods

… to `backupWorkingPatchList` and `restorePatchList`
Reference: sailfishos-patches#437 (comment)

* [patchmanagerobject.cpp] First round of name changes

* [patchmanagerobject.h] Carry out all name changes

* [patchmanager.cpp] Carry out all name changes

* [patchmanager.h] Carry out all name changes

* [org.SfietKonstantin.patchmanager.xml] Fix accidentally swapped names

* [PatchManagerPage.qml] Carry out all name changes

* [patchmanager-tool] Carry out all name changes

* [main.cpp] Carry out all name changes

¿Is the masking of the **"** with **\** correct in C++?

* [PatchManagerPage.qml] Enhance wording of menu entry

Better, but still not really good, IMO.  Issue is, it must be terse enough to fit in a single line in portrait orientation on a Jolla 1 (i.e., 540 pixels width)!

* [index.qdoc] Overhaul

Sorry, I became carried away: Originally I only adjusted the original lines 307 & 310, then started fixing a few typos, enhanced a few phrases etc. etc. and ended up in going over the whole file.

Open points:
- Line 110: "\uicontrol"
  Is this correct markup?  Does the sentence make sense this way?
- Line 161: "\badcode"
  Is this correct markup?
- Line 225: "In this state, all enabled services are disabled, and PM must be reactivated via the GUI."
  "Patches" instead of "services"?
- Line 289: "\warning"
  Not "Warning"?  I.e., is it a markup term?

* [patchmanagerobject.cpp] Complete name changes

Questions:
- Was capitalising "\c Patches" in line 372 O.K.?
- Is the "\sa" in line 373 correct?

* [patchmanagerobject.cpp] Try to rectify comments lines 344 - 387

Please review thoroughly!  It became obvious, that some copy&paste flaws existed, but I am not sure, if I rectified them correctly.  I simply assumed that the code is correct, but maybe (unlikely) the documentation mismatch was the other way around.

* [patchmanagerobject.cpp] Complete name changes & overhaul

Notes / questions:
- Line 587: "()"?  Function name missing?
- Line 1103: "on the bus" -> "on D-Bus"?
- Line 1198: Ambiguous, especially the second "all".
- Line 1705: Not better "/sa" instead of "See also"?
- Line 2121: Is that an accidentally deleted comment: Check `git blame`

* [patchmanagerobject.cpp] Revert over-eagerly applied capitalisation

* [index.qdoc] Two minor rectifications of yesterday's work

* [index.qdoc] Fix ambiguosity and missing "that" in line 120

* [index.qdoc] Enhance lines 124 and 135 a little bit

* [index.qdoc] More minor fixes

* [index.qdoc] Even moere small fixes

* [index.qdoc] More nitpicks

* [index.qdoc] Rectify broken sentence, detected and forgotten multiple times
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants