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

Added Pomodoro sample #161

Open
wants to merge 16 commits into
base: pending
from

Conversation

Projects
None yet
4 participants
@bitemyapp

bitemyapp commented Apr 1, 2018

I had a little bit of trouble figuring out some of the details of sharing and updating application state, so I think this could be a worthy augmentation of the clock.rs example.

I also added a Makefile because I got a bit confused by it defaulting to an old gtk feature flag and the build.sh seems to be purely for CI purposes, not for local builds. Accordingly, I also renamed build.sh to build-ci.sh for clearer intent.

zzeroo and others added some commits Apr 10, 2017

Add some spaces
To make the github rendering more attractive.
Merge pull request #128 from EPashkin/master
Moved all examples to `bin` subfolder
Merge pull request #144 from EPashkin/master
Merge pending to master
Merge pull request #157 from EPashkin/master
Merge branch 'pending'
@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 1, 2018

Thanks for PR.

Please follow https://github.com/gtk-rs/examples/blob/master/CONTRIBUTING.md. You can "Edit" this PR in Github page to change target branch (better do this after you do rebase locally and push new version).

If your sample need minimal version instead makefile you can use conditional build like in https://github.com/gtk-rs/examples/blob/master/src/bin/builder_basics.rs, and uncomment default = ["gtk_3_22"] locally (with change to required version).
As I see actual minimal version is 3.12 due set_margin_xx, so you better add gtk_3_12 to Cargo.toml and use it as switch.

Also gtk::Window::new as main window not recommended, please, use gtk::Application::new instead.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 1, 2018

At last: renaming build.sh is right thing, but this change will need update CI scripts in all your crates,
as CI depend on this name, so better don't do it in this PR.

const TOMA_MINUTES: i64 = 3;
const BREAK_MINUTES: i64 = 5;

const TOMA_MSG: &'static str = r###"

This comment has been minimized.

@EPashkin

EPashkin Apr 1, 2018

Member

no 'static needed: it default now

new_label.set_margin_top(0);
new_label.set_margin_bottom(0);
new_label.set_justify(gtk::Justification::Center);
return new_label

This comment has been minimized.

@EPashkin

EPashkin Apr 1, 2018

Member

Just new_label please


fn connect_click_start(tomaty: Rc<RefCell<Tomaty>>) {
let outer_tomato_heaven = tomaty.clone();
let ref button = outer_tomato_heaven.borrow().tomaty_button;

This comment has been minimized.

@EPashkin

EPashkin Apr 1, 2018

Member

let button = &outer_tomato_heaven.borrow().tomaty_button;

@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 1, 2018

Clippy reports many other warnings

bitemyapp added some commits Apr 1, 2018

@bitemyapp

This comment has been minimized.

bitemyapp commented Apr 2, 2018

@EPashkin I think I've made progress on cleaning this up.

Some of the other examples have clippy warnings too. I'm assuming you're not expecting me to clean up the other examples? Screenshot of cairo_threads clippy output:

screenshot from 2018-04-01 20-34-04

pomodoro.rs seems to be clippy clean now. Anything else?

@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 2, 2018

Thanks.
Yes I don't expecting clean warning in other files.

Under Windows 10 closing window will cause panic
thread 'main' panicked at 'Attempted to quit a GTK main loop when none is running.', gtk\src\rt.rs:163:13
Using ApplicationWindows (that better used instead ApplicationWindows not helps, and I currently don't have time to check more.

And, please, remove makefile it will be bother to maintain it (currently it don't have 3.12, 3.16) and it don't helps with running examples.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 2, 2018

And main problem that PR still against master branch, not pending

window.set_default_size(350, 70);

window.connect_delete_event(|_, _| {
gtk::main_quit();

This comment has been minimized.

@EPashkin

EPashkin Apr 2, 2018

Member

This need be window.destroy(); on clone

if gtk::init().is_err() {
println!("Failed to initialize GTK.");
return;
}

This comment has been minimized.

@EPashkin

EPashkin Apr 2, 2018

Member

This is unneeded as gtk::Application::new call it.

@bitemyapp bitemyapp changed the base branch from master to pending Apr 12, 2018

@bitemyapp

This comment has been minimized.

bitemyapp commented Apr 12, 2018

And, please, remove makefile it will be bother to maintain it (currently it don't have 3.12, 3.16) and it don't helps with running examples.

@EPashkin I lost time figuring out the correct syntax and features to build the apps and make it work. I will probably not be the last person to do so. Is there some other means of preventing other people wasting time on the same problem that would be acceptable to you?

I've solved the panic, removed the Makefile, and changed the PR target to pending.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Apr 12, 2018

Sorry, I don't understand what you means about "other means": we clearly stated in CONTRIBUTING.md that PR on master will be not merged (bad that Github seems don't allow force that),
other thing like using gtk::ApplicationWindow or make code look good (including git tree) is minor, and can be get from other examples.
Thanks, but currently too many changes and tree clogged.

Can you make new branch from pending (or just checkout pending),
add pomodoro.rs to it and change features in Cargo.toml,
and then change source branch here?
There also some nits last about pomodoro.rs, I will add it in code.

@GuillaumeGomez it almost time for you to look to this PR too.

fn build_ui(application: &gtk::Application) {
// let window = gtk::Window::new(gtk::WindowType::Toplevel);
let window = gtk::ApplicationWindow::new(application);
window.set_application(application);

This comment has been minimized.

@EPashkin

EPashkin Apr 12, 2018

Member

window.set_application(application); now unneeded and comment too.

window.set_default_size(350, 70);

window.connect_delete_event(clone!(window => move|_, _| {
window.destroy();

This comment has been minimized.

@EPashkin

EPashkin Apr 12, 2018

Member

Minor problem with clone macro: now it produce warning if build without features.
Please or move macro_rules! inside mod pomodoro
or remove it and change this to

let window_clone = window.clone();
window.connect_delete_event(move |_, _| {
    window_clone.destroy();

as it single macro usage

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