Renaming kde-plasma theme to KDE-Plasma breaks existing user configuration

[yan12125]

Expected Behavior

My favorite theme continues to work after upgrading from lxqt 1.0.0 to 1.1.0

Current Behavior

This line theme=kde-plasma in ~/.config/lxqt/lxqt.conf no longer works, and another theme is picked (ambiance in my case). I need to use theme=KDE-Plasma to bring my favorite theme back. (I believe most users use lxqt-config-appearance. Editing files directly is just my preference.)

Possible Solution

• Add a symlink kde-plasma -> KDE-Plasma?
• Migrate existing configurations automatically?

Steps to Reproduce (for bugs)

1. With LXQt 1.0.0, pick kde-plasma theme
2. Upgrading to 1.1.0

Context

Testing whether LXQt 1.1.0 explodes :)

Related: https://github.com/lxqt/lxqt-themes/pull/68.

System Information

• Distribution & Version: Arch Linux latest
• Kernel: 5.15.34-1-lts
• Qt Version: 5.15.3+kde+r137-1
• lxqt-build-tools Version: 0.11.0-1
• libqtxdg Version: 3.9.0-1
• Package version: 1.1.0-1

(All LXQt packages are built locally using Arch Linux PKGBUILDs)

[stefonarch]

This is expected so users will explore other themes ;) I think a symlink is the best solution, migration is an overkill IMO.

[tsujan]

migration is an overkill IMO.

I agree.

I think a symlink is the best solution

Will it work? I haven't tested.

[stefonarch]

Nope, realized after writing - users end up with both themes in lxqt-appearance. Maybe just leaving a note about that? It's not a big hassle but yes, it shouldn't happen.

[yan12125]

Nope, realized after writing - users end up with both themes in lxqt-appearance.

Yep, I only noticed that after testing

Maybe just leaving a note about that? It's not a big hassle but yes, it shouldn't happen.

A note sounds good!

[tsujan]

A more fundamental solution can be case-insensitivity in the code that's responsible for reading and applying the key. In LXQt Appearance Configuration → LXQt Theme, the first letters are already capitalized. So, maybe, we should have considered case-insensitivity from start.

[stefonarch]

We could also revert the renaming. I just didn't think about existing config break.

[tsujan]

We could also revert the renaming.

Then, the same problem will happen for a few git users that have already upgraded LXQt ;)

I think case-sensitivity is a problem, that has been ignored so far. If the theme names are supposed to be case-sensitive, why are they capitalized in the GUI?

[stefonarch]

A PR for this (also in the single announcement)?

[tsujan]

Yes, that's good for now.

[tsujan]

BTW, KDE-Plasma theme has the same problem that I worked around in Kvantum theme in https://github.com/lxqt/lxqt-themes/pull/64. The problem is in Qt's stylesheet support.

EDIT: I mean this:

[stefonarch]

So the first PR for 1.2.0 ;)

[tsujan]

→ https://github.com/lxqt/liblxqt/pull/308 (logging out and in is needed).

[yan12125]

We could also revert the renaming. I just didn't think about existing config break.

Maybe this is not a bad option. Only a few users are affected by double renaming. Other users, who will likely upgrade from 1.0.0 to 1.1.1 or newer, will not be affected.

In the future, to address the name issue mentioned in https://github.com/lxqt/lxqt-themes/issues/57, we may specify the name "KDE Plasma" in a file like /usr/share/lxqt/themes/kde-plasma/index.theme. By the way, the theme file can also be a basis towards translatable theme names (https://github.com/lxqt/lxqt/issues/455). For example, we can follow a similar pattern to XDG icon themes:

# /usr/share/lxqt/themes/kde-plasma/index.theme
[LXQt Theme]
Name=KDE Plasma
Name[uk]=KDE Плазма

(the Ukraine translation is copied from https://kde.org/uk/)

[stefonarch]

Sounds good, a more complete solution. Maybe a kind of .desktop/.yaml file as used for panel plugins, so it can contain also a description.

[yan12125]

Maybe a kind of .desktop/.yaml file as used for panel plugins, so it can contain also a description.

Good reminder! It's nice to adopt existing codes/scripts/infrastructure/workflow/... for the new file format. Just don't use .desktop file extension as they are not desktop entries ;)

[yan12125]

Seems the issue is not completely fixed. I just upgraded my other Arch machine to LXQt 1.1, and ambiance appeared again. lxqt.conf is updated by some program to have theme=ambiance as well. On Arch liblxqt is already patched with https://github.com/lxqt/liblxqt/pull/308, so I can get KDE-Plasma theme back if I revert lxqt.conf to have theme=kde-plasma. There are probably more things to change.

[stefonarch]

lxqt.conf is updated by some program ...

That's strange. There were 2 commits to change default icon theme and default theme, but this should'nt affect at all existing files in $HOME https://github.com/lxqt/lxqt-session/commit/5d81ae0c03bb3855841bc4b4c9a78160eaa38488

[tsujan]

Yes, more change is needed, in lxqt-session. I can make the needed changes as I did in https://github.com/lxqt/lxqt-config/pull/847, but there's a logical problem: the components shouldn't need to know about care-insensitivity. Actually, https://github.com/lxqt/lxqt-config/pull/847 isn't good either.

Instead, liblxqt may need a change and other components shouldn't use theme names for comparison. I'll look into it.

EDIT: lxqt-session doesn't need a change. See below.

[stefonarch]

I didn't test the newer versions with an upgrade in VM Ubuntu (upgrading using the ppa for 1.1.0 didn't change the theme as expected) but I see that they used a space in the filename:

ls -l /usr/share/lxqt/themes/
...
drwxr-xr-x 6 root root 4096 apr 21 10:24 'Lubuntu Arc'

[tsujan]

I just upgraded my other Arch machine to LXQt 1.1, and ambiance appeared again.

@yan12125, that may be inevitable. lxqt-session won't know about the new liblxqt until the session is restarted. I think this is what happens:

When the upgrade is done from inside an LXQt session, lxqt-themes is upgraded, the folder "kde-plasma" is removed and replaced by "KDE-Plasma", LXQtModuleManager::themeFolderChanged() (in lxqt-session/src/lxqtmodman.cpp) sees that the current theme path doesn't exist anymore, and so, it picks up the first theme, which is Ambiance.

[tsujan]

And, fortunately, lxqt-session doesn't need any change :) The only place where it checks the value of the "theme" key is inside LXQtModuleManager::themeFolderChanged(), and it's OK to forget about case-insensitivity there — actually, after a tiny PR I'll make for liblxqt, it'll be a little better to do so.

[yan12125]

that may be inevitable. lxqt-session won't know about the new liblxqt until the session is restarted

That sounds reasonable. If there is no way to fix it, are case-sensitivity PRs still needed? I prefer to handle inconsistency among folder names and theme names in a more a more generic way instead (ex: https://github.com/lxqt/lxqt-themes/issues/69#issuecomment-1102312158). Such a feature, which will be after 1.1.1, is useful for other use cases (ex: https://github.com/lxqt/lxqt/issues/455, which I mentioned earlier), and may get complicated with case-insensitive folder names.

[tsujan]

@yan12125 I continued the case-insensitivity PRs (there are two non-merged ones), partially because you approved the first PR. Personally, I think it's better to treat theme names case-insensitively and, because of your approval, I thought you had the same idea.

Since @palinek doesn't like it, if you've also changed your mind, I should reverse https://github.com/lxqt/liblxqt/pull/308 and close the non-merged PRs.

Please tell me what I should do! This is (and was) a matter of team decision.

[yan12125]

tsujan @.***> 於 2022年5月1日 週日 02:15 寫道：

@yan12125 https://github.com/yan12125 I continued the case-insensitivity PRs (there are two non-merged ones), partially because you approved the first PR. Personally, I think it's better to treat theme names case-insensitively and, because of your approval, I thought you had the same idea.

I have no strong opinion whether theme names should be case-insensitive or not. I approved it as I thought it could fix a regression, while it turns out not working in general cases.

Since @palinek https://github.com/palinek doesn't like it, if you've also changed your mind, I should reverse lxqt/liblxqt#308 https://github.com/lxqt/liblxqt/pull/308 and close the non-merged PRs.

Please tell me what I should do! This is (and was) a matter of team decision.

This is also a matter of team discussions besides team decisions :) Let me expand my last statement: I have a concern about code complexity if we want to keep case-insensitive theme names and adding features like translated theme names in the future. If that's the case (I guess so), I prefer a more generic approach to address inconsistency between theme folder names and displayed names and forget about case-insensitivity. If you think such code complexity will not happen, feel free to continue th work! I also checked the other two open PRs for case-insensitivity. They work as intended and the code logic is good, and both can be merged in the current form.

— Reply to this email directly, view it on GitHub https://github.com/lxqt/lxqt-themes/issues/69#issuecomment-1114031009, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOZCGN3C43UGWZ3AUKK6GTVHV2FFANCNFSM5TSQKUBA . You are receiving this because you were mentioned.Message ID: @.***>

[tsujan]

@yan12125

Thanks for your reply!

I have a concern about code complexity if we want to keep case-insensitive theme names and adding features like translated theme names in the future.

~Theme names cannot be translatable because they are names (→ https://github.com/lxqt/lxqt/issues/455#issuecomment-1114209300).~ See my next comment.

As for the code complexity, I don't think there's any. Of course, each feature needs extra codes. In this case, they are only https://github.com/lxqt/liblxqt/pull/308 (already merged), https://github.com/lxqt/liblxqt/pull/309 (a one-line change) and https://github.com/lxqt/lxqt-config/pull/848 (actually, a little simpler than the current master).

My main concern is different from yours. As I've mentioned before, lxqt-config already capitalizes theme names. That has always presupposed case-insensitivity. If we forget about case-insensitivity, we should remove capitalization. But, if we do so, we'll have an ugly list like:

ambiance
...
dark
frost
...

Therefore, sooner or later, we'll have to change those names. But that will be badly backward-incompatible and will break almost all theme settings after an update.

[tsujan]

Oh, I forgot the possibility of transliteration for theme names! Yes, theme names can be translatable but that's only about how they appear in the list, not how they're found by the code. I don't think it's related to case-insensitivity or case-sensitivity.

[yan12125]

that's only about how they appear in the list, not how they're found by the code

Got it, completely reasonable! Now I'm fine with continuing the work on case-insensitive themes names.

we'll have an ugly list like:

Although irrelevant now, let me share my earlier idea: before removing capitalization, there should be a way to specify displayed names for themes. For example, with a config file like

# /usr/share/lxqt/themes/ambiance/index.theme
[LXQt Theme]
Name=Ambianc

(This is similiar to an earlier example, while I changed it a little to avoid confusion)

The theme with name ambiance will be displayed as Ambiance. As a result, there will be no functionality or visible changes for users.

[tsujan]

before removing capitalization, there should be a way to specify displayed names for themes. For example,...

Very good point! It hadn't occurred to me. It removes my concern :) Thanks!

So, we could remove capitalization when implementing this feature, which will also be necessary for translation (or transliteration). In that way, there will be no need to case-insensitivity — the less code, the better.

If, after a day or two, no other reason for case-insensitivity comes to my mind, I'll reverse the PR in question and close the other two. Your solution seems much cleaner.

[tsujan]

Done. Now liblxqt is the same as 1.1.0.

We need to implement index.theme.

[yan12125]

Just noticed LXQt had a similar issue around folder name cases years ago: https://github.com/lxqt/lxqt/issues/500