-
-
Notifications
You must be signed in to change notification settings - Fork 75
Conversation
No check items and others? |
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)]); |
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.
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()
.
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.
Dialog still needed i32
not gtk::ResponseType
?
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.
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 :(
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.
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.
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.
May be it be error after rust-lang/rust#3868 ?
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.
Also make __Nonexhaustive(())
will break as i32
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.
Yes, we'll probably make __Nonexhaustive(())
.
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.
Then I make 3 PR's:
examples: change as i32
to .into()
for all generated enums,
gir: change generation,
gtk: regen
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.
👍
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.
window.show_all(); | ||
|
||
about.connect_activate(move |_| { | ||
let p = Dialog::new_with_buttons(Some("About"), |
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.
Aren't you supposed to use AboutDialog
here?
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.
I am.
@EPashkin: Happier? ;) |
@GuillaumeGomez rebase please, currently has |
Hm, now github as |
Looks great. |
Also closing by button not work in "About", seems it result of
|
In |
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!)")); |
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.
Come on, the license is MIT
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.
Okay, I put it...
GTK asks for a parent window and it's set in gtktest ( |
I changed copyright (less fun now...) and added parent window. |
About still has not working "License" and "Close" button, in "gtktest" license is hidden and close working. |
menu.append(&about); | ||
menu.append(&quit); | ||
file.set_submenu(Some(&menu)); | ||
menu_bar.append(&file); |
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 should try to use Glade (UI metadata editor). No one is expected to build UIs programmatically these days.
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.
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.
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.
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.
So, do we merge this PR? |
I'd very much like it if the menu (or the whole UI) would be defined in XML. |
And I said that I'd do it, but in a separated example. |
Actually, we should do this for all examples. |
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. |
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. |
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. |
And so that's why I proposed to have both ways. |
@GuillaumeGomez seems no changes to fixing my comment (not tested).
|
@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.
I wait for @gkoz to agree for this to be merged before modifying. |
Okay, let's have the manual example. You still need to make loading of the image work regardless of |
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. @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... |
Normally programmatic UI initialization isn't something one should want to accomplish and examples suggesting that approach distract from learning the idiomatic way.
Presumably using |
Updated. Do you still have the issues @EPashkin? |
Thanks, @GuillaumeGomez. |
AboutDialog, BoxExt, CheckMenuItem, ContainerExt, DialogExt, IconSize, Image, Inhibit, Label, | ||
Menu, MenuBar, MenuItem, MenuItemExt, MenuShellExt, WidgetExt, WidgetSignals, | ||
Window, WindowExt, WindowPosition, WindowType | ||
}; |
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 don't import traits one by one.
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.
I'll replace by gtk::prelude::*
after what we experienced recently.
Updated. |
Thanks! |
cc @gkoz