Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support kde6 panel floating #77

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Support kde6 panel floating #77

merged 2 commits into from
Mar 5, 2024

Conversation

mlyxshi
Copy link
Contributor

@mlyxshi mlyxshi commented Feb 29, 2024

In plasma-interactiveconsole, I try to print all object key of panel and find this key floating
#62

@toast003
Copy link
Collaborator

Thanks for finding this out @mlyxshi <3

It's not in the documentation yet

@toast003
Copy link
Collaborator

It doesn't work in plasma 5, so we should either say that it only works on 6, or set the panel to be floating like we do now if you're running plasma 5
Might take a look at it tomorrow

@mlyxshi
Copy link
Contributor Author

mlyxshi commented Feb 29, 2024

It doesn't work in plasma 5, so we should either say that it only works on 6, or set the panel to be floating like we do now if you're running plasma 5

It is a little bit tricky. Floating is one of the location in kde5, but a boolean key in kde6.

I don't know how to handle this transition. Personally, I have upgraded all my linux desktop to kde6 except steamdeck. This pr doesn't change the current kde panel configuration in my steamdeck, so I think it's backward compatible.

@toast003
Copy link
Collaborator

toast003 commented Mar 1, 2024

It is backwards compatible in the sense that it doesn't break anything in plasma 5, however I think that having two different ways to set a panel to floating is confusing.

Imo we should check if we're running on plasma 5 or 6, and then change location or floating depending on the version

@toast003
Copy link
Collaborator

toast003 commented Mar 1, 2024

@mlyxshi (or anyone that's running plasma6) can you run print(scriptingVersion) in plasma-interactiveconsole?

@mlyxshi
Copy link
Contributor Author

mlyxshi commented Mar 1, 2024

print(scriptingVersion)

20

I think that having two different ways to set a panel to floating is confusing.

It's originated from plasma... I'm also confused

@toast003
Copy link
Collaborator

toast003 commented Mar 1, 2024

Plasma 5 also has 20 so I'll have to use applicationVersion instead

It's originated from plasma... I'm also confused

My bad, I don't think I'm explaining myself properly 😅

Before plasma 6, if you wanted to make a floating panel you needed to set location to floating. Now there's a dedicated property for that ( the floating key you found).

If we add an option for floating just like you have here we could make a floating panel both by setting location to floating, and by setting floating to true.

What I think we should do remove the ability to set location to floating, and keep the floating option, and then use the key you found if you're running plasma 6, or the old way if not

Copy link
Collaborator

@magnouvean magnouvean left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

@@ -67,6 +67,7 @@ let
example = "autohide";
description = "The hiding mode of the panel.";
};
floating = lib.mkEnableOption "enable or disable floating style";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a disclaimer that it is plasma 6 only. Should also start with a capital letter and end with period as the rest of the descriptions

@toast003
Copy link
Collaborator

toast003 commented Mar 3, 2024

Yup same for me
We can implement what I said in a separate pr

@magnouvean
Copy link
Collaborator

One thing though I am not sure about is how we would determine if you are running plasma 5 or 6. Would it not be possible to have both installed at the same time? And even if it isn't would it be possible for plasma-manager which is built on top of home-manager to be able to check if plasma 6 or 5 is installed given that they are declared on the system-level

@toast003
Copy link
Collaborator

toast003 commented Mar 3, 2024

https://develop.kde.org/docs/plasma/scripting/api/#version-numbers

Starting with KDE SC 4.5, the version number of both the scripting API and the application is available to the script via the >following read-only properties:

string applicationVersion: the version of the application, e.g. 5.20.3
number scriptingVersion: the version of the scripting API; e.g. for Plasma 5.20 it is 20.

scriptingVersion is still 20 on plasma 6, but applicationVersion does change

@magnouvean
Copy link
Collaborator

Right, forgot that we can check for this in the script itself and not from nix. Sound like a good idea then!

@mlyxshi
Copy link
Contributor Author

mlyxshi commented Mar 4, 2024

Right, forgot that we can check for this in the script itself and not from nix. Sound like a good idea then!

Sounds great. I don't really want to go back to plasma5 and test floating stuff, because I don't use it anymore.
Another PR is welcome.

@magnouvean
Copy link
Collaborator

I understand that, tbh I am not overly keen to dive back and forth either, but will probably do some minimal testing to keep plasma 5 support somewhat alive. I reckon we can merge as soon as the above conversation about the description is resolved I reckon. The description could be for instance Enable or disable floating style (plasma 6 only).. We could just remove the plasma 6 only part when we implement plasma 5 support for the same option.

@mlyxshi
Copy link
Contributor Author

mlyxshi commented Mar 5, 2024

@magnouvean description updated

@magnouvean magnouvean merged commit 9bac592 into nix-community:trunk Mar 5, 2024
1 check passed
ugur-a added a commit to ugur-a/nixos-config that referenced this pull request Jul 7, 2024
turns out the one I used before was intended for Plasma 5
nix-community/plasma-manager#77
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.

3 participants