Skip to content

Conversation

@nephros
Copy link
Contributor

@nephros nephros commented Feb 25, 2022

Follow up on #272, more precisely #272 (comment)

Proposing this in draft mode for now.

  1. Do we want the menu option back? (I know I do use it occasionally but will be happy to maintain my private version of this is nothing for mainline PM.)
  2. This introduces a DConf value to be saved per-user. While this is normal for sailfish apps to do, it is a first for PM. Up until now all tunables were going form the UI -> Plugin -> Daemon -> /etc/patchmanager2.conf.
    This configuration value does not need to go through the daemon, as it doesn't need to know about it, it's UI-only. Also, that makes it multi-user ;)
    But it introdces a dep on Nemo.Configuration.

@nephros nephros marked this pull request as draft February 25, 2022 12:12
@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2022

  1. Oops, I obviously was not fully up to my usual quality standards, when I introduced what you fixed by commit cf87c44. Well, I know that the web-editor does add these superfluous spaces easily, but here I missed to pay attention. Thanks for noticing and rectifying.

  2. Do we want the menu option back?

    As mentioned before, I am fine with having "Deactivate all Patches" permanently at the topmost position of the pulley menu of Patchmanager's main page, see commit 272 for details (and and issue 20 for the debate before).

  3. I know I do use it occasionally but will be happy to maintain my private version of this is nothing for mainline PM.

    This approach does not make much sense to me:

    • If you want (or need) it, likely somebody else also does.
    • I appears to be easier (in terms of workload) to implement a generic solution once, than to maintain a private branch in the long run.
  4. This introduces a DConf value to be saved per-user. […]

    I expected this to be more intrusive, but it does not need that much code and does not look overly complicated.

    And the prospect of having a user specific configuration "framework" (well it is rather utilising what is already there), which is only evaluated by the PM GUI application (in contrast to the PM daemon, which uses and must use a system-wide configuration in /etc), which also may be utilised for other settings of the PM GUI application later, is very nice.

@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2022

Please check, that I denoted the technical details correctly when rephrasing the comment section for clarity:

/*
 * The usual, system-wide configuration values are set via D-Bus plugin by the
 * Patchmanager daemon, which stores them in /etc/patchmanager2.conf
 * This configuration group "uisettings" is for settings which *solely* affect
 * the PM GUI application and consequently also are per-user settings.
*/

While the string changes (just spelled out, what was meant / already there) bear much less risk of mishaps, please check them, too:
https://github.com/sailfishos-patches/patchmanager/pull/283/files#diff-c092975afb5d44c1002edfec699cb52c26bf8df373b6ed0696c0f8da182d889bR92-R93

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

AFAICT, this is reasonable and looking good.
Disclaimer: Please remember that "I am not a QML coder (IANAQC)!"

A single "Check & Balance" question: Is Nemo.Configuration 1.0 available in SailfishOS 3.4.0 (i.e., the oldest SFOS release supported by recent Patchmanager versions)?

… after trying "at" and pondering about "on" (both "feel" plausible, but not like a perfect fit).
@nephros
Copy link
Contributor Author

nephros commented Feb 26, 2022

A single "Check & Balance" question: Is Nemo.Configuration 1.0 available in SailfishOS 3.4.0 (i.e., the oldest SFOS release supported by recent Patchmanager versions)?

Definitely.
It has been the standard way of doing settings in QML for ages. And the Settings application uses it extensively.

The corresponding package name is nemo-qml-plugin-configuration-qt5, and it is documented at https://sailfishos.org/develop/docs/nemo-qml-plugin-configuration/

Now that you mention it though, it's possible that my spec file line calling for qml(Nemo.Configuration) is not resolved on older systems. Can you do a rpm -qP nemo-qml-plugin-configuration-qt5 and see if it lists that string?
If it causes problems we can either depend on the package name, or leave it out completely as it is safe to assume it will be present on any system PM runs on.

@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2022

Can you do a rpm -qP nemo-qml-plugin-configuration-qt5 and see if it lists that string?

It does, even on SFOS 3.2.1.

So this seems to be good to go! (Addressing: It is an "approved draft" currently.)

@nephros nephros marked this pull request as ready for review February 26, 2022 21:55
@nephros nephros merged commit a76de1f into sailfishos-patches:master Feb 26, 2022
@nephros nephros deleted the unapply-all branch February 26, 2022 22:35
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