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

Add support for file chooser buttons #602

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
4 participants
@demurgos
Contributor

demurgos commented Dec 3, 2017

This commit adds support for file chooser buttons. They are represented
as a slice of tuples (text, action). The original function uses
varargs to support an arbitrary number of arguments. This implementation
supports up to 3 buttons.

This is a breaking change.

See: https://developer.gnome.org/gtk3/stable/GtkFileChooserDialog.html#gtk-file-chooser-dialog-new

0 => ffi::gtk_file_chooser_dialog_new(title.to_glib_none().0, parent.to_glib_none().0, action.to_glib(), ptr::null::<c_char>()),
1 => ffi::gtk_file_chooser_dialog_new(title.to_glib_none().0, parent.to_glib_none().0, action.to_glib(), buttons[0].0.to_glib_none().0, buttons[0].1.to_glib(), ptr::null::<c_char>()),
2 => {
let second_button_text: *const c_char = buttons[1].0.to_glib_none().0;

This comment has been minimized.

@EPashkin

EPashkin Dec 3, 2017

Member

This won't work.
You need save Stash to variable and use .0 in function call.

This comment has been minimized.

@demurgos

demurgos Dec 3, 2017

Contributor

Thanks, I'll fix it.

This comment has been minimized.

@demurgos

demurgos Dec 3, 2017

Contributor

Fixed in the latest commit

let second_button_text: *const c_char = buttons[1].0.to_glib_none().0;
ffi::gtk_file_chooser_dialog_new(title.to_glib_none().0, parent.to_glib_none().0, action.to_glib(), buttons[0].0.to_glib_none().0, buttons[0].1.to_glib(), second_button_text, buttons[1].1.to_glib(), ptr::null::<c_char>())
},
_ => {

This comment has been minimized.

@sdroege

sdroege Dec 3, 2017

Member

This hard-coding of 4 different settings for the file chooser is also not ideal, and the usage of the enum prevents adding "custom" buttons to there. Not sure how to fix either of those though, the C API is kind of suboptimal in this regard.

This comment has been minimized.

@demurgos

demurgos Dec 3, 2017

Contributor

Fixed in the latest commit: instead of silently truncating the buttons, it panics. This is a temporary solution until variadic functions are supported.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

To remove "breaking change" you can just add this as "with_buttons" instead.
IMHO better panic or unimplemented for 4+ buttons.
Also there crate https://crates.io/crates/va_list

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 3, 2017

Thank you for the helpful reviews. I am not very familiar with these bindings but I'll fix these issues.

  • Save stash
  • Use with_buttons to prevent breaking change (ideally, there should only be one function: there is just one function in Gtk)
  • Use va_list or panic if the number of buttons is too high.
@sdroege

This comment has been minimized.

Member

sdroege commented Dec 3, 2017

The va_list crate only supports x86, x86-64 and arm, that's not a good idea. We'd need rust-lang/rfcs#2137 for solving this properly.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

@sdroege ResponseType still contains __Unknown(i32) so IMHO custom buttons is possible.

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 3, 2017

Ok, so I'll panic for now.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

@demurgos Please add TODO comment with link to rust-lang/rust#44930

demurgos added a commit to demurgos/gtk that referenced this pull request Dec 3, 2017

Improve file chooser with buttons reliability
Following the reviews in gtk-rs#602, this commit:
- uses the `with_buttons` function instead of modifying `new` to keep
  backward compatibility.
- saves the button texts in `Stash` objects to avoid invalid pointers
- panics when the number of buttons is too high (instead of silently
  truncating the number of buttons)
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 3, 2017

@EPashkin @sdroege I updated my PR following your comments. Could you take a look at it?

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

Please, remove this comment, I will add it to right place (https://github.com/gtk-rs/lgpl-docs) after PR merged

    /// This function creates a `FileChooserDialog` with buttons:
    /// ```
    /// let dialog = FileChooserDialog::with_buttons::<ApplicationWindow>(
    ///   Option::Some("Open File"),
    ///   Option::None,
    ///   FileChooserAction::Open,
    ///   &[("_Cancel", ResponseType::Cancel), ("_Open", ResponseType::Accept)]
    /// );
    /// ```
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 3, 2017

@EPashkin Done, I removed the doc comment.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

@demurgos I will change doc' example to this. It works for you?

let dialog = FileChooserDialog::with_buttons::<Window>(
  Some("Open File"),
  None,
  FileChooserAction::Open,
  &[("_Cancel", ResponseType::Cancel), ("_Open", ResponseType::Accept)]
);
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 3, 2017

Yes, this is fine. I was using ApplicationWindow and just copy pasted, but you are right that it is better to simply use Window.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 3, 2017

cc @sdroege and @GuillaumeGomez for final decision.

title.to_glib_none().0,
parent.to_glib_none().0,
action.to_glib(),
first_button_text.0,

This comment has been minimized.

@sdroege

sdroege Dec 3, 2017

Member

Why not just buttons[0].0.to_glib_none().0 here?

This comment has been minimized.

@demurgos

demurgos Dec 3, 2017

Contributor

For consistency with the other branches.

To be honest, I also do not really understand how Stash works. I just know that when I used a single stash, I had some errors with pointers and adding more stashes fixed it.

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 3, 2017

Looks good to me except for the comment I just added. Feel free to ignore me on that though if there's a reason

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 3, 2017

I wrote a va_list crate which works on every OS/arch. It's used by a VLC demuxer so I suppose it's fine enough. Maybe give it a look?

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 6, 2017

@sdroege
I replied to your comment. Is it fine to leave stashes for consistency or do I have to remove it. If I have to remove it, do I have to remove every first_button_text variable or just the one in the "one button" case?

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 6, 2017

If it's not needed, it seems simpler/shorter to not define new variables for the Stash but just directly use the value

demurgos added a commit to demurgos/gtk that referenced this pull request Dec 7, 2017

Remove extraneous local variable in `FileChooserDialog::with_buttons`
This commit removes the `first_button_text` variable from
`FileChooserDialog::with_buttons`, as requested in the PR gtk-rs#602
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 7, 2017

@sdroege
I removed the variables for the text of the first button. I tested it locally once for 1, 2 and 3 buttons (it works fine).

It is still not clear for me why I can omit the stash for the first button but if I try to do it for the others it breaks.

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 7, 2017

The CI error seems to be caused by the usage example. Travis tries to compile it even if it's not a complete program.

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 7, 2017

How does it break for the other buttons? It should work in all cases

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 7, 2017

I get the wrong text, instead of printing the text of the button 2, it reuses the window title as button labels. I'll try to reproduce it, it may have been an error on my side.

@EPashkin EPashkin referenced this pull request Dec 7, 2017

Merged

Fix doc test #40

demurgos added a commit to demurgos/gtk that referenced this pull request Dec 7, 2017

Remove local variable in `FileChooserDialog::with_buttons`
This commit removes the Stash variables from
`FileChooserDialog::with_buttons`, as requested in the PR gtk-rs#602

demurgos added a commit to demurgos/gtk that referenced this pull request Dec 7, 2017

Remove local variables in `FileChooserDialog::with_buttons`
This commit removes the Stash variables from
`FileChooserDialog::with_buttons`, as requested in the PR gtk-rs#602
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 7, 2017

I was not able to reproduce my issue (wrong text when not using the Stash) so I assume the error was on my side. I pushed the version without any local variables.

action.to_glib(),
buttons[0].0.to_glib_none().0,
buttons[0].1.to_glib(),
(buttons[1].0.to_glib_none() as Stash<*const c_char, str>).0,

This comment has been minimized.

@EPashkin

EPashkin Dec 7, 2017

Member

Stash IMHO don't needed here and in other places

This comment has been minimized.

@EPashkin

EPashkin Dec 7, 2017

Member

Forgot that it var_arg :( so rust can't infer type

This comment has been minimized.

@demurgos

demurgos Dec 7, 2017

Contributor

Exactly, I can omit it for the first text but not for the other ones.

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 8, 2017

Seems good now?

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 8, 2017

It's good for me.

@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 11, 2017

@EPashkin @sdroege
Is it possible to advance with this PR?

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 11, 2017

All good from my side

panic!(format!("`FileChooserDialog::with_buttons` does not support 4+ buttons, received {}", buttons.len()))
}
})
.downcast_unchecked()

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 11, 2017

Member

Please put this call on the previous line, right after }).

This comment has been minimized.

@demurgos

demurgos Dec 11, 2017

Contributor

Fixed in the latest commit.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 11, 2017

Well now please squash your commits. Otherwise seems good to me.

Add support for file chooser buttons
This commit adds support for file chooser buttons. They are represented
as a slice of tuples `(text, action)`. The original function uses
varargs to support an arbitrary number of arguments. This implementation
supports up to 3 buttons.

**This is a breaking change.**

See: https://developer.gnome.org/gtk3/stable/GtkFileChooserDialog.html#gtk-file-chooser-dialog-new

Improve file chooser with buttons reliability

Following the reviews in #602, this commit:
- uses the `with_buttons` function instead of modifying `new` to keep
  backward compatibility.
- saves the button texts in `Stash` objects to avoid invalid pointers
- panics when the number of buttons is too high (instead of silently
  truncating the number of buttons)

Remove doc comment for `FileChooserDialog::with_buttons`

Remove local variables in `FileChooserDialog::with_buttons`

This commit removes the Stash variables from
`FileChooserDialog::with_buttons`, as requested in the PR #602

Fix formatting issue

Keep `downcast_unchecked` method on the same line as the closing
parenthesis of the chopped-down call to `Widget::from_glib_none`.
@demurgos

This comment has been minimized.

Contributor

demurgos commented Dec 11, 2017

@GuillaumeGomez
Done, I squashed the commits, it should be ready now 😄

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 11, 2017

Just waiting confirmation from @EPashkin.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 11, 2017

@demurgos Thanks.
@GuillaumeGomez LGFM

@GuillaumeGomez GuillaumeGomez merged commit edc7f50 into gtk-rs:master Dec 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@demurgos demurgos deleted the demurgos:file-chooser-buttons branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment