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 constants publicly accessible #237

Open
oakes opened this issue Mar 20, 2015 · 13 comments
Open

Make constants publicly accessible #237

oakes opened this issue Mar 20, 2015 · 13 comments

Comments

@oakes
Copy link
Contributor

oakes commented Mar 20, 2015

On request, I am making this issue for the discussion about button constants.

@GuillaumeGomez
Copy link
Contributor

Thanks ! cc @gkoz

@gkoz
Copy link
Contributor

gkoz commented Mar 20, 2015

It seems there's no precedent for exporting such constants in rgtk. Maybe an enum would be better.
What if we renamed the trait to AsDialogButtons and introduced

enum DialogButtons {
    Ok,
    OkCancel,
    YesNo,
    // etc
}

impl AsDialogButtons for DialogButtons {
    // ...
}

then exported this enum?

@GuillaumeGomez
Copy link
Contributor

I like what you did with your constants. @jeremyletang and I tried as much as possible to keep enums when possible but there are places where it causes problems (in big enums but I don't which ones). So don't worry about that.

@gkoz
Copy link
Contributor

gkoz commented Mar 20, 2015

The enum would be used pretty much the same way as the consts but avoid the ugly uppercased names :)

SomeDialog::new("title", DialogButtons::YesNo)

As an unexpected benefit it would let us replace the GtkButtonsType buttons parameter of MessageDialog::new and improve consistency between different dialogs while pretending it works just like the underlying API. https://developer.gnome.org/gtk3/stable/GtkMessageDialog.html#gtk-message-dialog-new

An interesting detail: the Gnome HIG recommends against using such generic constants (I guess that's why the dialogs take variable button arrays now) and calls for placing the Cancel button first.
https://developer.gnome.org/hig/stable/dialogs.html.en#primary-buttons

@GuillaumeGomez
Copy link
Contributor

Then it's perfect ! Let's do it (or rather please do it !) this way !

I didn't know that. We could add a little doc above the function with this link for users to give gnome recommendations.

@gkoz
Copy link
Contributor

gkoz commented Mar 21, 2015

Ugh, I'm starting to feel that the variadic hack is just not worth it. It won't matter if add_button is called a couple times. So I'm gonna scrap the abomination. The Dialog APIs can get much better then.

@GuillaumeGomez
Copy link
Contributor

@jeremyletang and I were thinking about doing it with a macro (but I think I already told you that, no ?). But it wasn't a priority so we didn't do it.

@gkoz
Copy link
Contributor

gkoz commented Mar 21, 2015

I couldn't figure out a way to use a macro without exposing it to the users which is totally not worth it. Maximizing efficiency of adding dialog buttons shouldn't be a priority anyway right? :)

@gkoz
Copy link
Contributor

gkoz commented Mar 21, 2015

Actually all gtk_dialog_add_buttons does is call gtk_dialog_add_buttons_va_list which calls gtk_dialog_add_button in a loop. We'll actually improve efficiency by doing the loop ourselves. Using those variadic functions officially Doesn't Make Sense.
So yeah.

@gkoz
Copy link
Contributor

gkoz commented Mar 23, 2015

So after pondering Dialog and its associated types more, @GuillaumeGomez would you be open to trying out a different convention whereas the dialog module would be moved out of traits to live directly under gtk and contain all the dialog related enums (currently named ResponseType, MessageType, ButtonsType) as well as DialogTrait and Dialog, AboutDialog and MessageDialog (other dialogs are a bit more ambiguous because they come in pairs of e.g. ColorChooserDialog/ColorChooserWidget).
The Response enum would incorporate the GtkResponseType variants and User(u16) variant, the Buttons enum more or less as described above.

Motivation: Rust seems to favour utilizing hierarchical modules organization against dumping all types in a flat namespace. This more of a self-documenting approach would benefit new GTK users although might be confusing to those, who've used other GTK bindings. The benefit of physical separation of the widgets/traits modules on the other hand is still lost on me, it hurts maintainability and requires having more modules and files than necessary.

@GuillaumeGomez
Copy link
Contributor

I'm not very open to this but I can't find any good argument against it so I think we should give it a try. If this seems fine, then we'll have to extend it to other similar cases.

@gkoz
Copy link
Contributor

gkoz commented Mar 23, 2015

Okay, I'll make a PR to see how this scheme works out in practice.

@GuillaumeGomez
Copy link
Contributor

Perfect ! Thanks for that. I'm curious to see the result.

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

No branches or pull requests

3 participants