Skip to content

Conversation

@SoNiC-HeRE
Copy link
Contributor

Added Screenshot Demo in a new section Portal under Library
Issue workbenchdev/demos#3

@SoNiC-HeRE SoNiC-HeRE marked this pull request as draft March 5, 2023 20:11
@SoNiC-HeRE SoNiC-HeRE marked this pull request as ready for review March 5, 2023 20:11
@SoNiC-HeRE SoNiC-HeRE marked this pull request as draft March 5, 2023 20:14
@sonnyp sonnyp changed the title Added Screenshot Demo using Libportal library: Add libportal screenshot entry Mar 6, 2023
@sonnyp sonnyp self-requested a review March 11, 2023 22:59
@sonnyp sonnyp self-assigned this Mar 11, 2023
@sonnyp
Copy link
Contributor

sonnyp commented Mar 13, 2023

So this is working fine for me but

Gdk-Critical: gdk_wayland_toplevel_export_handle: assertion '!wayland_toplevel->xdg_exported' failed

For @SoNiC-HeRE - on the same OS (Fedora 37) - I'm investigating what it could be.

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Mar 20, 2023

The error has been resolved but when i click the button nothing happens

@sonnyp sonnyp force-pushed the main branch 3 times, most recently from 05f69f1 to 55e450f Compare May 21, 2023 10:37
@sonnyp
Copy link
Contributor

sonnyp commented May 28, 2023

@SoNiC-HeRE I merged main into this branch. Could you check once more if this still doesn't work for you?

Perhaps check Workbench permissions

image

@sonnyp sonnyp assigned SoNiC-HeRE and unassigned sonnyp May 28, 2023
@SoNiC-HeRE
Copy link
Contributor Author

Sure, I'll check again

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented May 28, 2023

@SoNiC-HeRE I merged main into this branch. Could you check once more if this still doesn't work for you?

Perhaps check Workbench permissions

image

Works fine now, had to install it seperately to check, probably was missing some sort of permission when run directly from builder

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

2 more things

Please use Gio._promisify

Can you check if you get an error if the permission is denied? If so we can display a message using Adw.MessageDialog telling the user how to fix it (I'll let you come up with a clear and concise sentence).

Comment on lines 3 to 4
const { GdkPixbuf, Gio, GLib, GObject, Gst, Gtk, Pango, PangoCairo } =
imports.gi;
Copy link
Contributor

Choose a reason for hiding this comment

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

please use import x from y

Comment on lines 46 to 49
Adw.PreferencesGroup library_portal {
title: "User Interface";
}

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe

Suggested change
Adw.PreferencesGroup library_portal {
title: "User Interface";
}
Adw.PreferencesGroup library_desktop {
title: "Desktop APIs";
}

we can move Notifications there later

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, even better (but it's mobile too)

Suggested change
Adw.PreferencesGroup library_portal {
title: "User Interface";
}
Adw.PreferencesGroup library_platform {
title: "Platform APIs";
}


Adw.StatusPage {
title: "Screenshot";
description: _("Keep a snapshot of your current screen");
Copy link
Contributor

Choose a reason for hiding this comment

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

It should address the developer, not the end user.

Suggested change
description: _("Keep a snapshot of your current screen");
description: _("Take a picture of the screen");


Button Screenshot {
icon-name: "camera";
margin-bottom: 40;
Copy link
Contributor

Choose a reason for hiding this comment

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

please use multiples of 6

Suggested change
margin-bottom: 40;
margin-bottom: 42;


LinkButton {
label: "API Reference";
uri: "https://libportal.org/";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uri: "https://libportal.org/";
uri: "https://libportal.org/method.Portal.take_screenshot.html";

});
}

Screenshot.connect("clicked", _takeScreenshot);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Screenshot.connect("clicked", _takeScreenshot);
button.connect("clicked", takeScreenshot);

{
"name": "Screenshot",
"category": "portal",
"description": "Keep a snapshot of your current screen",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Keep a snapshot of your current screen",
"description": "Take a picture of the screen",

@@ -0,0 +1,10 @@
{
"name": "Screenshot",
"category": "portal",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"category": "portal",
"category": "desktop",

"category": "portal",
"description": "Keep a snapshot of your current screen",
"panels": [
"ui",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"ui",
"code",

it's more important here

Comment on lines 19 to 25
const pixbuf = GdkPixbuf.Pixbuf.new_from_file_at_scale(
path,
60,
40,
true,
);
workbench.builder.get_object("picture").set_pixbuf(pixbuf);
Copy link
Contributor

Choose a reason for hiding this comment

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

hehe thankefully we don't need pixbuf and scale to arbitrary dimensions (which don't work equally well for differentt display rattios)

take_screenshot gives us a file:/// uri, which we can use to load the file directly into Picture

const uri = ...
const file = Gio.File.new_for_uri(uri);
picture.set_file(file);

@SoNiC-HeRE SoNiC-HeRE marked this pull request as ready for review May 29, 2023 16:02
@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented May 29, 2023

@sonnyp The portal methods doesnt seem to work properly with GIO's async methods. Tried implementing without using Gio._promisify

Also i thought It'd be better if we could directly log the error message instead of Adw.messagedialog wdyt?

@sonnyp
Copy link
Contributor

sonnyp commented May 29, 2023

The portal methods doesnt seem to work properly with GIO's async methods

What is the error? What's not working? I can take a look.

Also i thought It'd be better if we could directly log the error message instead of Adw.messagedialog wdyt?

The MessageDialog is not for showing the error, it's to tell the user what to do if the permission is/was denied.

If error is permission denied: show a MessageDialog to tel the user what todo
If error is not permission denied: logError

@SoNiC-HeRE
Copy link
Contributor Author

The portal methods doesnt seem to work properly with GIO's async methods

What is the error? What's not working? I can take a look.

Also i thought It'd be better if we could directly log the error message instead of Adw.messagedialog wdyt?

The MessageDialog is not for showing the error, it's to tell the user what to do if the permission is/was denied.

If error is permission denied: show a MessageDialog to tel the user what todo If error is not permission denied: logError

I'll try fixing again , if not i'll share

@SoNiC-HeRE
Copy link
Contributor Author

All of the above mentioned changes have been implemented. Implemented with Async method as well but works fine even without it. Thought code would be shorter and cleaner.

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Good error handling 👍

An issue I noticed while testing the demo is that the size and position of elements change once the screenshot is taken and displayed. It doesn't feel great.

I have fixed that, made promisify work and address the last comments in this commit: 0f22390

Please fetch Workbench main and look at my changes and the result.

Comment on lines 18 to 21
icon-name: "screenshooter-symbolic";
margin-bottom: 42;
width-request: 6;
height-request: 48;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't override stanard elements default allocation (size).

Also, I think a label button would be clearer. No need for small icon button.

const portal = new Xdp.Portal();
const parent = XdpGtk.parent_new_gtk(workbench.window);
const picture = workbench.builder.get_object("picture");
const window = button.get_ancestor(Gtk.Window);
Copy link
Contributor

Choose a reason for hiding this comment

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

that's workbench.window

Comment on lines 8 to 12
const button = workbench.builder.get_object("button");
const portal = new Xdp.Portal();
const parent = XdpGtk.parent_new_gtk(workbench.window);
const picture = workbench.builder.get_object("picture");
const window = button.get_ancestor(Gtk.Window);
Copy link
Contributor

Choose a reason for hiding this comment

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

try to group things that make sense together

Comment on lines 33 to 34
picture.width_request = 180;
picture.height_request = 180;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remember to use properties in Bluepint when possible

@sonnyp sonnyp merged commit 62cc7ac into workbenchdev:main May 30, 2023
@SoNiC-HeRE
Copy link
Contributor Author

Good error handling +1

An issue I noticed while testing the demo is that the size and position of elements change once the screenshot is taken and displayed. It doesn't feel great.

I have fixed that, made promisify work and address the last comments in this commit: 0f22390

Please fetch Workbench main and look at my changes and the result.

Sure, I'll have a look

sonnyp pushed a commit to krlade/Workbench that referenced this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants