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

feat: Implement set_wallpaper_uri method in Wallpaper.Portal #3

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

anudeeps0306
Copy link

Description:

This pull request implements the set_wallpaper_uri method in the Wallpaper.Portal class, providing functionality to set the wallpaper.

Copy link
Member

@kgilmer kgilmer left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for contributing to Regolith @anudeeps0306 . Can you describe test steps if I want to try this out myself?

try {
get_uri = Uri.parse(uri, UriFlags.NONE).get_path();
} catch (GLib.UriError e) {
stderr.printf("Error parsing URI: %s\n", e.message);
Copy link
Member

Choose a reason for hiding this comment

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

question: why swallow exception and continue? this seems like a critical error, shouldn't we re-throw or at least exit the function early?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

src/Wallpaper/Portal.vala Show resolved Hide resolved
src/Wallpaper/Portal.vala Outdated Show resolved Hide resolved
@anudeeps0306
Copy link
Author

Nice! Thanks for contributing to Regolith @anudeeps0306 . Can you describe test steps if I want to try this out myself?

@kgilmer You can test it using the ASHPD demo application. In this application, navigate to the wallpaper section to check its functionality.

In Ubuntu, after building the xdg backend for Regolith, you can test it. This involves using FileChooser.OpenFile of GTK, where the implementation of xdg Regolith's backend Wallpaper.Portal will be utilized.

However, there might be an issue in testing it within Regolith due to a problem with config ordering (flatpak/xdg-desktop-portal#1240). Currently, the FileChooser.OpenFile implementation of the xdg Regolith is used instead of the GTK backend. We'll need to wait for this issue to be resolved and merged for it to function properly.

Handling swallow exception of get_uri.
@kgilmer
Copy link
Member

kgilmer commented Mar 16, 2024

Thanks for the test setup instructions @anudeeps0306 . I installed ashpd via flatpak. I build and run your version on my computer after killing the prod version. I then launch ashpd demo and choose "Select a File". I noticed tried uninstalled the gtk portal implementation based on your notes. However it seems that the background portal requires the file portal that the gtk portal was providing. Doesn't seem like there is a way to test end to end at this time, which is ok. Do you plan to implement all portals so that we do not need to fall back to gtk, or something else?

Copy link
Member

@kgilmer kgilmer left a comment

Choose a reason for hiding this comment

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

LGTM, add comment about return code values if you like

@anudeeps0306
Copy link
Author

Thanks for the test setup instructions @anudeeps0306 . I installed ashpd via flatpak. I build and run your version on my computer after killing the prod version. I then launch ashpd demo and choose "Select a File". I noticed tried uninstalled the gtk portal implementation based on your notes. However it seems that the background portal requires the file portal that the gtk portal was providing. Doesn't seem like there is a way to test end to end at this time, which is ok. Do you plan to implement all portals so that we do not need to fall back to gtk, or something else?

I dicussed about it with @SoumyaRanjanPatnaik. If the implementation is functioning in GTK, we can utilize it directly. We'll develop backends for cases where GTK isn't operational

Adding comments for response codes.

if (show_preview) {
try{
show_preview_image(get_uri,parent_window);
Copy link
Contributor

Choose a reason for hiding this comment

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

We would probably want to wait for confirmation from the user before proceeding to set the wallpaper.

src/Wallpaper/Portal.vala Outdated Show resolved Hide resolved
src/Wallpaper/Portal.vala Outdated Show resolved Hide resolved
src/Wallpaper/Portal.vala Outdated Show resolved Hide resolved
try {
get_uri = Uri.parse(uri, UriFlags.NONE).get_path();
} catch (GLib.UriError e) {
stderr.printf("Error parsing URI: %s\n", e.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

stdout.printf("old_line: %s", old_line);


contentsString = contentsString.replace( old_line, line);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, but my preference would be to comment the old line and set the resource values in a new line. Also delete any existing comments to avoid cluttering up the xresources.

@SoumyaRanjanPatnaik
Copy link
Contributor

The changes mostly look good to me now. Although not a priority, but see if you can make the preview window look a little better.

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