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

hyprpm: target installed instead of running version #8634

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

RoootTheFox
Copy link
Contributor

Describe your PR, what does it fix/add?

After updating Hyprland, hyprpm update will still build plugins against the currently running version of Hyprland.
This is a little annoying, because you first have to restart Hyprland and then run hyprpm update for your plugins to be built for the correct version.
This PR makes hyprpm build against the installed version of Hyprland instead of the running version, and shows a popup notification if the running Hyprland version differs from the installed one after updating, telling users to restart Hyprland.
example

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

When I tested it on my setup (Arch) everything worked fine, so no I don't think so. Not sure how Nix handles stuff, but it should work there too I suppose.
I did change the signature of hyprpm's getHyprlandVersion func, but gave it a default value of true that corresponds to the original behavior.

Is it ready for merging, or does it need work?

Should be ready for merging, although if you have any feedback (e.g. UX wise) go for it

@github-actions github-actions bot added the hyprpm label Dec 3, 2024
@vaxerski
Copy link
Member

vaxerski commented Dec 4, 2024

sooo... essentially then it builds for the installed version right?

We also need a safeguard in enable for this, don't we?

Signed-off-by: rooot <hey@rooot.gay>
@RoootTheFox
Copy link
Contributor Author

sooo... essentially then it builds for the installed version right?

yeah.

We also need a safeguard in enable for this, don't we?

i mean, the actual safeguard is in ensurePluginsLoadState which is used by enable too where (from my understanding) the actual plugin loading (and unloading) happens

i suppose a notify would be neat though (like how i have it impld in update)

i'll go and update upstream rq and then impl that

hyprpm/src/main.cpp Outdated Show resolved Hide resolved
hyprpm/src/main.cpp Outdated Show resolved Hide resolved
hyprpm/src/core/PluginManager.cpp Outdated Show resolved Hide resolved
Signed-off-by: rooot <hey@rooot.gay>
@RoootTheFox
Copy link
Contributor Author

somehow missed the other 2 snake_case vars i still had, oops
if you want i can squash the commits into one single commit too

@vaxerski
Copy link
Member

vaxerski commented Dec 8, 2024

I always squash dw

hyprpm/src/core/PluginManager.cpp Outdated Show resolved Hide resolved
hyprpm/src/core/PluginManager.cpp Show resolved Hide resolved
Signed-off-by: rooot <hey@rooot.gay>
@RoootTheFox RoootTheFox requested a review from vaxerski December 9, 2024 19:59
hyprpm/src/core/PluginManager.cpp Outdated Show resolved Hide resolved
hyprpm/src/core/PluginManager.cpp Outdated Show resolved Hide resolved
Signed-off-by: rooot <hey@rooot.gay>
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

rest lgtm

hyprpm/src/main.cpp Outdated Show resolved Hide resolved
hyprpm/src/main.cpp Outdated Show resolved Hide resolved
Signed-off-by: rooot <hey@rooot.gay>
@RoootTheFox
Copy link
Contributor Author

fixed :)

@vaxerski vaxerski merged commit 33f271c into hyprwm:main Dec 11, 2024
11 checks passed
@vaxerski
Copy link
Member

@RoootTheFox not just for me but the message pops up every hyprpm update

I don't think that's intended
image

@RoootTheFox
Copy link
Contributor Author

yeah, oops, that's definitely not intended. i'll look into fixing that once i get time

@vaxerski
Copy link
Member

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants