Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Add menu bar example #88

Merged
merged 1 commit into from
Apr 29, 2016
Merged

Add menu bar example #88

merged 1 commit into from
Apr 29, 2016

Conversation

GuillaumeGomez
Copy link
Member

cc @gkoz

@EPashkin
Copy link
Member

No check items and others?

@GuillaumeGomez
Copy link
Member Author

I wanted to make this example as light as possible but maybe it's too light now haha.

let p = Dialog::new_with_buttons(Some("About"),
Some(&window),
gtk::DIALOG_MODAL,
&[("Ok", gtk::ResponseType::Ok as i32)]);
Copy link
Member

Choose a reason for hiding this comment

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

Huh. I guess we've got a problem: a breaking change, which doesn't actually break the build. This new enum wasn't supposed to be convertible via as i32 but it still is. Might have to make the __Nonexhaustive member non-empty to prevent this.
The proper conversion would be ResponseType::Ok.into().

Copy link
Member

Choose a reason for hiding this comment

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

Dialog still needed i32 not gtk::ResponseType?

Copy link
Member

Choose a reason for hiding this comment

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

It supports arbitrary response codes and until ResponseType accommodates that it needs i32. Additionally, gir doesn't have a facility to substitute ResponseType for i32 where we want it to :(

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, right now ResponseType::Ok as i32 == 4 while for the type formerly known as ResponseType it's gtk_sys::GtkResponseType::Ok as i32 == -5 and that is what .into() will return.

Copy link
Member

Choose a reason for hiding this comment

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

May be it be error after rust-lang/rust#3868 ?

Copy link
Member

Choose a reason for hiding this comment

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

Also make __Nonexhaustive(()) will break as i32

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we'll probably make __Nonexhaustive(()).

Copy link
Member

Choose a reason for hiding this comment

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

Then I make 3 PR's:
examples: change as i32 to .into() for all generated enums,
gir: change generation,
gtk: regen

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

window.show_all();

about.connect_activate(move |_| {
let p = Dialog::new_with_buttons(Some("About"),
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you supposed to use AboutDialog here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am.

@EPashkin EPashkin mentioned this pull request Apr 25, 2016
@GuillaumeGomez
Copy link
Member Author

@EPashkin: Happier? ;)

@EPashkin
Copy link
Member

EPashkin commented Apr 25, 2016

@GuillaumeGomez rebase please, currently has IconSize::Button as i32

@EPashkin
Copy link
Member

Hm, now github as New changes since you last viewed

@EPashkin
Copy link
Member

Looks great.
May be better hide "License" button in "About" as it empty?

@EPashkin
Copy link
Member

EPashkin commented Apr 25, 2016

Also closing by button not work in "About", seems it result of

Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.

@EPashkin
Copy link
Member

EPashkin commented Apr 25, 2016

In gtktest all normal with About.

p.set_website_label(Some("gtk-rs"));
p.set_website(Some("http://gtk-rs.org"));
p.set_comments(Some("What a menu bar example!"));
p.set_copyright(Some("All rights reserved to us (of course!)"));
Copy link
Member

Choose a reason for hiding this comment

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

Come on, the license is MIT

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I put it...

@gkoz
Copy link
Member

gkoz commented Apr 25, 2016

In gtktest all normal with About.

GTK asks for a parent window and it's set in gtktest (set_transient_for).

@GuillaumeGomez
Copy link
Member Author

I changed copyright (less fun now...) and added parent window.

@EPashkin
Copy link
Member

About still has not working "License" and "Close" button, in "gtktest" license is hidden and close working.
And rebase please

menu.append(&about);
menu.append(&quit);
file.set_submenu(Some(&menu));
menu_bar.append(&file);
Copy link
Member

Choose a reason for hiding this comment

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

We should try to use Glade (UI metadata editor). No one is expected to build UIs programmatically these days.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone can use Glade. I think it's a good thing to show how it works programatically. However, we could provide all examples in Glade version.

Copy link
Member

Choose a reason for hiding this comment

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

There's a chance we're unaware of some obstacles to using Glade, especially around menus and actions. The GTK learning materials push strongly toward using Glade, doing a lot of these manipulations manually seems unidiomatic. So we need to prioritize making sure the Glade way works.

@GuillaumeGomez
Copy link
Member Author

So, do we merge this PR?

@gkoz
Copy link
Member

gkoz commented Apr 28, 2016

I'd very much like it if the menu (or the whole UI) would be defined in XML.

@GuillaumeGomez
Copy link
Member Author

And I said that I'd do it, but in a separated example.

@GuillaumeGomez
Copy link
Member Author

Actually, we should do this for all examples.

@gkoz
Copy link
Member

gkoz commented Apr 28, 2016

Frankly, teaching users to do this stuff manually is a disservice. This is not what they need to learn to be efficient at making GTK apps.

@GuillaumeGomez
Copy link
Member Author

All the other examples have been written by hand. This one isn't an exception. The point of these examples is to show how do it, either they want to do it by using GLADE or not.

@gkoz
Copy link
Member

gkoz commented Apr 28, 2016

I'm afraid this hasn't been how to do it for years now. We just sort of weren't paying attention. It's unfortunate that the existing examples don't match the modern ways but it's not an excuse for not learning new things and keeping rehashing the old stuff.

@GuillaumeGomez
Copy link
Member Author

And so that's why I proposed to have both ways.

@EPashkin
Copy link
Member

@GuillaumeGomez seems no changes to fixing my comment (not tested).

"About" still has not working "License" and "Close" button, in "gtktest" license is hidden and close working.
And rebase please

@EPashkin
Copy link
Member

@gkoz direct way generating allow to show how to use library, so I prefer have only one Glade example and other same that this.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Apr 28, 2016

@gkoz direct way generating allow to show how to use library, so I prefer have only one Glade example and other same that this.

You're more extreme than me! ;) Providing an example in both GLADE and full "hand-made" source code seems a good compromise.

@GuillaumeGomez seems no changes to fixing my comment (not tested).

I wait for @gkoz to agree for this to be merged before modifying.

@gkoz
Copy link
Member

gkoz commented Apr 28, 2016

Okay, let's have the manual example. You still need to make loading of the image work regardless of $CWD.

@EPashkin
Copy link
Member

I changed my options on using glade: goal of examples is show how do some task and better that example concentrated to this task, so if you need do some complex preparation then better use any way to simplify it.
But my option about this example still unaffected as example for working with menu.

@gkoz Can we include image to code as resource at compile time and use it?

@GuillaumeGomez
Copy link
Member Author

@gkoz Can we include image to code as resource at compile time and use it?

It's possible, I did it in C++. However I have no clue how to do so in Rust...

@gkoz
Copy link
Member

gkoz commented Apr 28, 2016

goal of examples is show how do some task and better that example concentrated to this task

Normally programmatic UI initialization isn't something one should want to accomplish and examples suggesting that approach distract from learning the idiomatic way.

Can we include image to code as resource at compile time and use it?

Presumably using glib-compile-resources and then doing something like this should work. We might consider making a convenience function to achieve that with less boilerplate.

@GuillaumeGomez
Copy link
Member Author

Updated. Do you still have the issues @EPashkin?

@EPashkin
Copy link
Member

Thanks, @GuillaumeGomez.
All good except picture, but it can be done later, as it better changed to many examples.

AboutDialog, BoxExt, CheckMenuItem, ContainerExt, DialogExt, IconSize, Image, Inhibit, Label,
Menu, MenuBar, MenuItem, MenuItemExt, MenuShellExt, WidgetExt, WidgetSignals,
Window, WindowExt, WindowPosition, WindowType
};
Copy link
Member

@gkoz gkoz Apr 28, 2016

Choose a reason for hiding this comment

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

We don't import traits one by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll replace by gtk::prelude::* after what we experienced recently.

@GuillaumeGomez
Copy link
Member Author

Updated.

@gkoz
Copy link
Member

gkoz commented Apr 29, 2016

Thanks!

@gkoz gkoz merged commit cfc782c into gtk-rs:pending Apr 29, 2016
@GuillaumeGomez GuillaumeGomez deleted the pending branch April 29, 2016 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants