-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@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 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 |
Handling swallow exception of get_uri.
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 |
There was a problem hiding this 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
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.
src/Wallpaper/Portal.vala
Outdated
|
||
if (show_preview) { | ||
try{ | ||
show_preview_image(get_uri,parent_window); |
There was a problem hiding this comment.
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.
try { | ||
get_uri = Uri.parse(uri, UriFlags.NONE).get_path(); | ||
} catch (GLib.UriError e) { | ||
stderr.printf("Error parsing URI: %s\n", e.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
…ig file to set wallpaper key for regolith desktop
src/Wallpaper/Portal.vala
Outdated
stdout.printf("old_line: %s", old_line); | ||
|
||
|
||
contentsString = contentsString.replace( old_line, line); |
There was a problem hiding this comment.
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.
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. |
… one's will be added
Description:
This pull request implements the
set_wallpaper_uri
method in theWallpaper.Portal
class, providing functionality to set the wallpaper.