[patchmanager.cpp] Fix-up release 3.2.10 ⇒ 3.2.11

[Olf0]

This was introduced by https://github.com/sailfishos-patches/patchmanager/commit/e30292be22fc6644e8b5ba32ac9612395bb11aff#diff-df38830cae3d3051528c63a2740fe3b0b4ec2c6b59aadab35dce5a1b6e995dd4R1016-R1030. I still do not understand the purpose of this addition, specifically in the light of the comment(s) stating "probably dead code, need to investigate": It sure is dead code, as these two newly added classes here do not contain any code at all. Likely this is a combination of my lack of understanding and these comments lacking information needed for me to understand the purpose of this addition.

Another point to investigate: Why did earlier CI runs not show this compile failure?

[nephros]

Sorry about that, I probably lapsed in a conflict merge or so.

[nephros]

Nah, so what happens is that qdoc warns about undocumented classes and methods.

But one can only document in .cpp files, not header files. So I tried what happens if I add a stub/empty implementation.

Apparently a stupid move - (although it does work wrt. generating documentation).

[nephros]

It's probably best to remove everything including the doc comment and live with the qdoc complaints for now.

[Olf0]

I still do not understand the purpose of this addition, specifically in the light of the comment(s) stating "probably dead code, need to investigate": It sure is dead code, as these two newly added classes here do not contain any code at all.

Nah, so what happens is that qdoc warns about undocumented classes and methods. But one can only document in .cpp files, not header files. So I tried what happens if I add a stub/empty implementation.

O.K., thank you for explaining the reason for these two empty method stubs. I will document that, if …

Apparently a stupid move - (although it does work wrt. generating documentation).

… we can settle on a proper way to make the compiler and qdoc happy.

By the help of your explanation, I also also see now, that creating these two dummy methods must follow the corresponding object definitions in /src/qml/patchmanager.h in class PatchManager: public QObject { :

• public slots: void activation(const QString & patch, const QString & version);
• signals: void easterReceived(const QString & easterText);

It's probably best to remove everything including the doc comment and live with the qdoc complaints for now.

With an strong emphasis on "for now", I do concur, so I can prepare a release now.

But in general I do not think it was "apparently a stupid move", it just lacked a introductory comment line stating:

/*
The only purpose of the following two empty methods 
is to document their corresponding definitions in 
/src/qml/patchmanager.h, because qdoc warns about 
undocumented classes and methods, but does not allow for 
qdoc source documentation in header files.  Apparently 
these two definitions are not used anywhere; now they are, 
by these two method stubs, and also documented here.
*/

After having written this, I start to see your point: Why not let qdoc correctly warn about these definitions being unused? I think a possible answer is: Because nobody perceives these warnings in automated, workflow driven runs at GitHub. Another one may be: Does "treat warnings as errors" also apply to qdoc runs; i.e., does this potentially break the GH workflow? (I assume the answer is "No".)

Furthermore, your commit db2e8f8 contradicts the little C++ know-how I (believe to) have: Why would that need a semicolon, specifically at this location? And if it is really required, why only for the second stub, but not also the first one?

P.S.: Note that it compiled fine after each commit in this PR, even though I cannot believe the code was correct in all these intermediate states, which brings me back to …

Another point to investigate: Why did earlier CI runs not show this compile failure?

[Olf0]

@nephros, are you O.K. with these three, little changes in comments: https://github.com/sailfishos-patches/patchmanager/pull/451/files#diff-a43ca147b12960521ec92e86439aecdb612da40b4573a5f65f62dc4430e70d22 If so, please re-approve.

Edit: Arrgh, this is not correct:

• The signal easterReceived is used in src/qml/AboutPage.qml!
• When searching for the private function activation, the only hit was in /src/bin/patchmanager-daemon/patchmanagerobject.cpp, which on first sight looks to me as a false positive, correct?

_{I should not try to seriously review C++ code: with so little know-how, the process and its result is hell for me and the readers of my "analyses": Convoluted, insecure etc.}

I will not try to decide how to proceed tonight, after all this back and forth: Do you have a reasonable suggestion at hand (take your time, quick shots may backfire, as we have seen here)?

---

Less important side notes:

1. P.S.: Note that it compiled fine after each commit in this PR, though I cannot believe the code was correct in all these intermediate states, which brings me back to …

   Another point to investigate: Why did earlier CI runs not show this compile failure?

   I got further on this front: By stubbornly walking through the logs of prior CI on PRs runs (relative to the failed CI on tags run), I discovered that their compilation also failed, but this failure was not propagated to a negative result of the whole job. I faintly remember this weirdness mentioned somewhere in the Actions documentation, at least I now know what to look for and implement in order to let the CI on PR runs become meaningful.

2. A likely irrelevant observation GitHub's editor in GitHub's web-frontend does some nice syntax-highlighting, at places even more than highlighting, but this is highly context dependent: While editing it does some extra-highlighting, when not editing it additionally shows classes, functions, variables etc., which are clickable and reveal a lot of information this way, in a side-bar at the right. Though the latter is the much mightier tool (only superseded by GitHub's VS-Code editor, but this runs quite slowly, due to consisting of a pile of JavaScript executed locally), what caught my eye was the extra syntax highlighting when editing (i.e., this is not visible when not editing), which marked three statements in orange (and solely these three): I wonder, if this really means something or if it is just erratic extra-highlighting in orange.
   •  
   •  

[nephros]

About qdoc warnings failing a CI run: no it doesn't.

The -Werror option only applies to gcc, not qdoc. And qdoc is currently not run in the CI action.

[nephros]

About the semicolon:

The failing snippet is:

void foo(bar);
{
}

which is a syntax error or at least worth a warning. db2e8f8 removes the semicolon. We then would get a warning (->error) about the unused parameter bar.

[Olf0]

This release is dreaded: Now the CI run failed with an "out of disk space" error for the first time.

I will have to split the steps, which use different docker images, into multiple jobs. :frowning:

---

Luckily the first rounds of compilation went fine, so tested compiling PM 3.2.11 in my PM-testing repo at the SFOS-OBS and ultimately in SailfishOS:Chum:testing, too.

@b100dian and @nephros, I would appreciate a little testing of Patchmanager 3.2.11 from SailfishOS:Chum:testing very much, despite it comprises a set of seemingly "uncritical" changes (see the first four words of this message).

[Olf0]

This release is dreaded: Now the CI run failed with an "out of disk space" error for the first time.

I will have to split the steps, which use different docker images, into multiple jobs. 😦

Done by PR #454.

---

Luckily the first rounds of compilation went fine, so tested compiling PM 3.2.11 in my PM-testing repo at the SFOS-OBS and ultimately in SailfishOS:Chum:testing, too.

@b100dian and @nephros, I would appreciate a little testing of Patchmanager 3.2.11 from SailfishOS:Chum:testing very much, despite it comprises a set of seemingly "uncritical" changes (see the first four words of this message).

After a few days have passed, I dared to submit Patchmanager 3.2.11 to the SailfishOS:Chum repository proper. AFAICS, no complaints, yet.