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

Implement new gs_plugin_*_repo functions and increase repo generalization #20

Merged
merged 5 commits into from
Dec 28, 2021

Conversation

pabloyoyoista
Copy link
Collaborator

@pabloyoyoista pabloyoyoista commented Dec 18, 2021

Helps #13

I believe this is the last bit pending to have the same functionality in GNOME 41 that there existed in GNOME 3.38. So maybe after merging this one would be the moment for a new release?

There is one caveat to the current implementation though. Disabling a repository behaves the same as removing it. Therefore, if somebody disables a repo, closes the dialog and reopens it, the repository will not show (instead of the logical showing as disabled). This problem can be avoided not implementing the enable/disable functions. In that case the switches will be blocked, but the removal button will still exist. I do not have a clear mind here on which of the two options are better. Ideas?

The underlying reason for this problem is that (as far as I understand) apk does not really differentiate between installed/enable and removed/disabled repos. The implemenation in apk-polkit-rs is super nice in the fact that considers a disabled repo as a repo which is commented out. Unfortunately that detail is only presented in the DBus as a read property, but there is no method to disable or enable a repository. Looking at the code, implementing it should be fairly simple, but I have no clue about rust, so I don't think I'll be able to implement such functionality soon. Anyway, I created https://gitlab.alpinelinux.org/Cogitri/apk-polkit-rs/-/issues/9 to track it.

BTW, I have tested this patch in multiple combinations, and it all looks to be working quite alright :)

@Cogitri
Copy link
Owner

Cogitri commented Dec 23, 2021

There is one caveat to the current implementation though. Disabling a repository behaves the same as removing it. Therefore, if somebody disables a repo, closes the dialog and reopens it, the repository will not show (instead of the logical showing as disabled). This problem can be avoided not implementing the enable/disable functions. In that case the switches will be blocked, but the removal button will still exist. I do not have a clear mind here on which of the two options are better. Ideas?

if we don't properly support enabling/disabling repos, we shouldn't implement those functions for now IMHO

@Cogitri
Copy link
Owner

Cogitri commented Dec 23, 2021

CI should be fixed (and way faster) with #24

@pabloyoyoista
Copy link
Collaborator Author

if we don't properly support enabling/disabling repos, we shouldn't implement those functions for now IMHO

Ok yeah, does not look clean. It's fixed now. One more excuse to learn rust on my side :)

Also, it looks like the credential was not properly substituted this time... I'll check if I have to enable something.

We have no idea of the length the strings are supposed to have
apk-polkit-rs does not support disabling repositories,
but only installing and removing. There is some sort
of support, as repos are identified enabled and disabled,
although that property cannot be written by using the
DBus interface.
@pabloyoyoista
Copy link
Collaborator Author

Wonderful, CI is fixed now!!!! There has been at least two people testing all the changes included in the repo since v0.8.2 from the artifacts in alpine's MR (which also include this PR) . Dylan and Mighty. I would say that that after merging this, it should be safe to do a release and get it updated in alpine :)))

@Cogitri Cogitri merged commit 10441ba into Cogitri:master Dec 28, 2021
@Cogitri
Copy link
Owner

Cogitri commented Dec 28, 2021

Pushed v0.9.0

@pabloyoyoista pabloyoyoista deleted the repo-fix branch December 29, 2021 18:22
@pabloyoyoista
Copy link
Collaborator Author

Thanks a lot!!!

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