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

Make basic UI #1

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Make basic UI #1

merged 1 commit into from
Oct 9, 2018

Conversation

GuillaumeGomez
Copy link
Collaborator

@GuillaumeGomez
Copy link
Collaborator Author

We should maybe a license as well, no?

@sdroege
Copy link
Owner

sdroege commented Sep 30, 2018

We should maybe a license as well, no?

Yes, I'd vote MIT as that's about as liberal as it gets

src/main.rs Outdated
p.set_website_label(Some("github repository"));
p.set_website(Some("https://github.com/sdroege/rustfest-rome18-gtk-gst-workshop"));
p.set_comments(Some("A process viewer GUI wrote with gtk-rs"));
// p.set_copyright(Some("This is under MIT license"));
Copy link
Owner

Choose a reason for hiding this comment

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

Yes :)

src/main.rs Outdated
p.set_authors(&["Sebastian Dröge", "Guillaume Gomez"]);
p.set_website_label(Some("github repository"));
p.set_website(Some("https://github.com/sdroege/rustfest-rome18-gtk-gst-workshop"));
p.set_comments(Some("A process viewer GUI wrote with gtk-rs"));
Copy link
Owner

Choose a reason for hiding this comment

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

Not a process viewer ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Woups. :-.

menu_bar.append_submenu("?", &about_menu);

// Connect the about menu entry click event.
let about = gio::SimpleAction::new("about", None);
Copy link
Owner

Choose a reason for hiding this comment

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

Modern GTK apps usually put this (and global preferences, etc) into the global per application menu, not inside the application. And inside the application there's usually a hamburger menu on the right for window-specific settings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Never heard of it and my searches gave nothing. Is it a gtk/gio thing?

Copy link
Owner

Choose a reason for hiding this comment

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

I thought that's what the gio::MenuItem stuff is already doing but apparently not. If you run gnome-shell and open e.g. gedit, you'll have various of it's global menu items in the gnome-shell menu. And then various window-specific ones in the hamburger menu at the top-right (save as, find, etc.).

I'll research for the global menu tomorrow :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's weird... Waiting more info on this. :D

Copy link
Owner

Choose a reason for hiding this comment

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

diff --git a/src/main.rs b/src/main.rs
index 00eea79..2992d52 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -21,13 +21,10 @@ macro_rules! upgrade_weak {
 
 fn build_menu(application: &gtk::Application, window: &gtk::ApplicationWindow) {
     let menu = gio::Menu::new();
-    let menu_bar = gio::Menu::new();
-    let about_menu = gio::Menu::new();
-
     menu.append("Quit", "app.quit");
 
-    about_menu.append("About", "app.about");
-    menu_bar.append_submenu("?", &about_menu);
+    let about_menu = gio::MenuItem::new("About", "app.about");
+    menu.append_item(&about_menu);
 
     // Connect the about menu entry click event.
     let about = gio::SimpleAction::new("about", None);
@@ -37,13 +34,15 @@ fn build_menu(application: &gtk::Application, window: &gtk::ApplicationWindow) {
         let p = gtk::AboutDialog::new();
         p.set_authors(&["Sebastian Dröge", "Guillaume Gomez"]);
         p.set_website_label(Some("github repository"));
-        p.set_website(Some("https://github.com/sdroege/rustfest-rome18-gtk-gst-workshop"));
+        p.set_website(Some(
+            "https://github.com/sdroege/rustfest-rome18-gtk-gst-workshop",
+        ));
         p.set_comments(Some("A process viewer GUI wrote with gtk-rs"));
         // p.set_copyright(Some("This is under MIT license"));
         p.set_transient_for(Some(&window));
+        p.set_modal(true);
         p.set_program_name("Rustfest GTK+GStreamer");
-        p.run();
-        p.destroy();
+        p.show_all();
     });
     let quit = gio::SimpleAction::new("quit", None);
     let weak_window = window.downgrade();
@@ -56,7 +55,6 @@ fn build_menu(application: &gtk::Application, window: &gtk::ApplicationWindow) {
     application.add_action(&quit);
 
     application.set_app_menu(&menu);
-    application.set_menubar(&menu_bar);
 }
 
 fn build_ui(application: &gtk::Application) {

src/main.rs Outdated
p.set_transient_for(Some(&window));
p.set_program_name("Rustfest GTK+GStreamer");
p.run();
p.destroy();
Copy link
Owner

Choose a reason for hiding this comment

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

I'd recommend not doing a recursive main loop here. Just do p.show() and for destroying you can connect to signals on the dialog. See the listbox example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the recommended way to do it.

Copy link
Owner

Choose a reason for hiding this comment

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

Where does it say so? :)

The problem with this is that you can very easily run into multiple mutable borrows of a RefCell for example by doing this. As while the dialog is running, everything (incl all other signal handlers) are called from inside p.run().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the examples but I'll update it if you prefer, I don't really mind. :)

Copy link
Owner

Choose a reason for hiding this comment

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

See #1 (comment)

In the context of Rust I'd really recommend to not do recursive main loops. RefCell will easily panic on you then if you're not careful, and there's not really any advantage (unlike in C) because we have cheap and ergonomic closures

extern crate gtk;

use gio::prelude::*;
use gio::MenuExt;
Copy link
Owner

Choose a reason for hiding this comment

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

Why is MenuExt no in the gio prelude?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a pretty good question!

Copy link
Owner

Choose a reason for hiding this comment

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

We should fix that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed but that won't get in before rustfest I assume.

Copy link
Owner

@sdroege sdroege Sep 30, 2018

Choose a reason for hiding this comment

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

Not a new release, yes. Unless you do a bugfix release for that (I do bugfix releases for such things and lots of other stuff in gstreamer-rs regularly as it makes users happy and is not much effort, see https://github.com/sdroege/gstreamer-rs/blob/master/gstreamer/CHANGELOG.md)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's say I'll think about. :-.

@GuillaumeGomez
Copy link
Collaborator Author

Updated, sorry for the delay!

src/main.rs Outdated
p.set_website(Some(
"https://github.com/sdroege/rustfest-rome18-gtk-gst-workshop",
));
p.set_comments(Some("A video input reader written with gtk-rs and gstreamer-rs"));
Copy link
Owner

Choose a reason for hiding this comment

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

"A webcam viewer written ..." seem better :)

src/main.rs Outdated
p.set_copyright(Some("This is under MIT license"));
p.set_transient_for(Some(&window));
p.set_modal(true);
p.set_program_name("Rustfest GTK+GStreamer");
Copy link
Owner

Choose a reason for hiding this comment

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

RustFest with capital F AFAIU?

src/main.rs Outdated
fn build_ui(application: &gtk::Application) {
let window = gtk::ApplicationWindow::new(application);

window.set_title("Rustfest 2018 GTK+GStreamer");
Copy link
Owner

Choose a reason for hiding this comment

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

RustFest

src/main.rs Outdated
application.connect_startup(|app| {
build_ui(app);
});
application.connect_activate(|_| {});
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we have this empty signal handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because of:

(.:91070): GLib-GIO-WARNING **: 08:58:30.931: Your application does not implement g_application_activate() and has no handlers connected to the 'activate' signal. It should do one of these.

I've removed it anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

Then keep it, warnings are not good :) We should probably do something in activate though?

I assume activate should be the one that creates the UI, while startup only cares for the menu registration and stuff? Or maybe activate should only call window.show_all()?

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe we shouldn't use startup at all? It sounds like that signal is mostly for internal reasons to decide if the application is already running or not?

Copy link
Owner

Choose a reason for hiding this comment

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

Asked on the GTK IRC channel, the docs are not clear at all

@sdroege
Copy link
Owner

sdroege commented Oct 9, 2018

Thanks, just minor stuff missing. Just merge after those are done :)

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.

None yet

2 participants