Skip to content

Conversation

@SoNiC-HeRE
Copy link
Contributor

Closes #390

@SoNiC-HeRE
Copy link
Contributor Author

Kooha-2023-07-16-20-04-06.webm

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jul 16, 2023

I had another idea for Uri Launcher to dynamically change uri using a file from FileChooser and getting its uri similar to file launcher; however when i use this : uri_launcher.set_uri(file.get_uri());
I get the warning:

Gjs-WARNING **: 20:21:56.432: JS ERROR: Gtk.DialogError: The application launch failed

and it does'nt open the file

@andyholmes
Copy link
Contributor

...and it does'nt open the file

This is probably because the Gtk.UriLauncher doesn't know to add the file to the documents portal, but it's hard to say. If I set it to a URL it seems to work fine.

@SoNiC-HeRE
Copy link
Contributor Author

...and it does'nt open the file

This is probably because the Gtk.UriLauncher doesn't know to add the file to the documents portal, but it's hard to say. If I set it to a URL it seems to work fine.

Is the current demo fine? or should i try fixing the above error? An early review might help me with it

@andyholmes
Copy link
Contributor

Is the current demo fine? or should i try fixing the above error? An early review might help me with it

I think just remove that part.

I'm only guessing why that's happening, but we should encourage developers to use Gtk.FileLauncher for any file-related operations since they're almost always going to be local and restricted by any sandboxing that's active.

Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Looking good!

As usual, I've got some "opinions" about the UI, but I think you can get this one finished in the next run :)

@SoNiC-HeRE SoNiC-HeRE requested a review from andyholmes August 3, 2023 12:52
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Lots of cleanups here, to keep the demo focused on the launcher APIs.

The only thing left I'd really like to cleanup is the "Launch"/"Open Folder" buttons, which should be associated with each other.

Comment on lines 39 to 48
Box {
styles ["linked"]

Button file_details {
label: _("Open File");
}
Button file_location {
label: _("Open Folder");
}
}
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
Box {
styles ["linked"]
Button file_details {
label: _("Open File");
}
Button file_location {
label: _("Open Folder");
}
}
Button file_location {
label: _("Open Folder");
}

Removing the "Open File", which conflicts with the meaning of the "Launch" button. The "Open Folder" folder button is a good idea, but I think it should be somehow connected or associate with the "Launch" button; any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought a workaround for this; I changed the open file to file name instead so that it doesnt conflict with launch button; I had another idea how about renaming this to File details wherein i log the details of the file such as size, name etc

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just not sure what purpose it serves, and it seems a bit confusing that it is presented with the same importance as "Open Folder".

"Launch" and "Open Folder" both represent methods of the Gtk.FileLauncher class, while the other button simply prints the file name to the console. Maybe instead the file selection button could have a caption with the current file name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's what I would do:

  • Move the constructors for both launchers to their respective sections in the code view.
  • Connect to notify::file and query the file name to set a caption on the "Choose another file" button
  • Set the default file and URI after the signals are connected, so they populate at first run
  • Make an equally prominent button for "View"

Here is what I'd imagine that looking like:

Screencast.from.2023-08-05.16-52-36.webm

@SoNiC-HeRE SoNiC-HeRE requested a review from andyholmes August 5, 2023 16:36
@SoNiC-HeRE SoNiC-HeRE requested a review from andyholmes August 7, 2023 19:05
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Looks good, but the "Open Folder" still needs to be emphasized as part of the Gtk.FileLauncher class.

The "File Name" button could just be a label instead, and the "Open Folder" button placed beside the "Launch" button.

@SoNiC-HeRE SoNiC-HeRE requested a review from andyholmes August 9, 2023 19:07
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Nicely done!

@andyholmes andyholmes merged commit 32eb95d into main Aug 9, 2023
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Aug 13, 2023
* Adds launcher entry

* Updated code

* Clean Up

* Applied changes

* Updated demo

* Suggested Changes

* New property
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.

Add a GtkFileLauncher and GtkUriLauncher Demo

3 participants