Review

[ArekPiekarz]

I'm sending a a review what could be improved in this project, in random order of importance.

1. We should include Cargo.lock file in binary projects to have reproducible builds. If it was a library project it would be better not to include it, so that the users of the library could decide in their binary projects the exact versions of dependencies.

2. The code below is not needed in Rust edition 2018, it was probably needed only in 2015.

   extern crate cairo;
   extern crate gtk;
   extern crate gio;
   extern crate glib;
   extern crate rand;
   extern crate chrono;

3. These manually written operators:

   
   impl std::ops::AddAssign for EuclideanVector {
   fn add_assign(&mut self, other: EuclideanVector) {
       self.dx += other.dx;
       self.dy += other.dy;
   }
   }

impl std::ops::Mul for EuclideanVector { type Output = Self;

fn mul(self, scalar: f64) -> Self {
    Self{dx: self.dx * scalar, dy: self.dy * scalar}
}

}

impl std::ops::Div for EuclideanVector { type Output = Self;

fn div(self, scalar: f64) -> Self {
    Self{dx: self.dx / scalar, dy: self.dy / scalar}
}

}

could be generated automatically using crate [derive_more](https://crates.io/crates/derive_more) like this:
```toml
# in Cargo.toml
[dependencies]
# ...
derive_more = { version = "0.99.9", default-features = false, features = ["add_assign", "mul"]}

// in main.rs
use derive_more::{AddAssign, Div, Mul};

#[derive(AddAssign, Copy, Clone, Div, Mul)]
struct EuclideanVector {
    dx: f64,
    dy: f64,
}

The only distadvantages are 1 more dependency and a little longer compile times from scratch. On my laptop they were:

debug mode:

• original: 1m 23s
• with derive_more: 1m 24s

release mode:

• original: 2m 12s
• with derive_more: 2m 18s

4. We can use for x in &container instead of for x in container.iter(). For mutable access, it would be for x in &mut container instead of for x in container.iter_mut(). Another alternative is container.iter().for_each(|x| x.doAction());, but it's a matter of preference.

5. We don't need to use std::ptr::eq here, because we can just compare references. In Rust they are not automatically dereferenced like in C++. This code:

   
   use std::ptr;

impl std::cmp::PartialEq for Body { fn eq(&self, other: &Self) -> bool { ptr::eq(self, other) } }

would become:
```rust
impl std::cmp::PartialEq for Body {
    fn eq(&self, other: &Self) -> bool {
        self == other
    }
}

6. We could get rid of storing Situation in Rc\ by using channels to react to GTK events. The basic idea is we clone the senders and move them into closures (lambdas) used by GTK, like connect_draw(). In those closures we use the senders to send our custom event to the main event queue, which then is forwarded to the receiver. There is only one receiver and we attach a closure that handles all our events. This way the Situation needs to be stored only in the last closure.

Example pseudocode:

const DEFAULT_CONTEXT : Option<&glib::MainContext> = None;
enum Event {
    Draw,
    Timeout
}

// in main()
let model = Situation::new()...;
let (sender, receiver) = glib::MainContext::channel(glib::PRIORITY_DEFAULT);
let sender2 = sender.clone();
drawing_area.connect_draw(move |...| {
    sender.send(Event::Draw);
});
gtk::timeout_add(time, move || {
    sender2.send(Event::Timeout);
});

receiver.attach(DEFAULT_CONTEXT, move |event| {
    match event {
        Event::Draw => onDraw(&model),
        Event::Timeout => onTimeout(&model)
    }
});

I noticed that drawing requires the drawing area and Cairo context, so you may need to send them in the event, if that is possible. Maybe glib::MainContext::sync_channel will be required.

7. Using crate itertools we can turn this code:

   application.run(&args().collect::<Vec<_>>());

   into this:

   # in Cargo.toml
   [dependencies]
   # ...
   itertools = "0.9.0"

   // in main.rs
   use itertools::Itertools;
   // ...
   application.run(&args().collect_vec());

8. After removing "extern crates" from main.rs and running cargo +nightly udeps it revealed that dependencies rand and png are not used and can be removed.

9. We can further optimize dependencies by disabling default features and enabling only the ones we need:

   [dependencies]
   cairo-rs = { version = "0.9.1", default-features = false}
   gtk = {version = "0.9.1", default-features = false}
   gio = {version = "0.9.0", default-features = false}
   glib = {version = "0.10.1", default-features = false}
   chrono = {version = "0.4.13", default-features = false, features = ["clock"]}

   This has the following effects:

   • lowers the number of dependencies from 144 to 123
   • lowers build times from 1m 23s to 1m 20s in debug mode and from 2m 12s to 2m 09s in release mode.

10. We can run cargo clippy to find more things to improve: cargo clippy -- -W clippy::pedantic -W clippy::style -W clippy::complexity -W clippy::correctness -W clippy::perf -W clippy::nursery I didn't enable warning groups -W clippy::restriction and -W clippy::cargo. For example (real output gives more warnings):

    
    warning: unnecessary structure name repetition
    --> src/main.rs:58:37
    |
    58 |     fn add_assign(&mut self, other: EuclideanVector) {
    |                                     ^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
    |
    = note: `-W clippy::use-self` implied by `-W clippy::nursery`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self

warning: this could be a const fn --> src/main.rs:97:5	97	/ pub fn new() -> Body { 98		Body{ 99		position: Coordinate{x: 0., y:0.}, 100		mass: 0., ...	104		} 105		}		_____^

= note: `-W clippy::missing-const-for-fn` implied by `-W clippy::nursery`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn

[szdanowi]

Whoa, awesome. Thanks for your effort - it means a lot. :+1:

Let me respond to your points one by one:

1. Don't know atm what Cargo.lock is - need to take a look at it and then I will try to make use of it.
2. Hm, okay, will try to remove these. Apparently I've been using 2015 Rust tutorial and examples then.
3. Cool - less noise and more actual code :+1:
4. Actually I was a bit annoyed by doing this .iter() call on each for loop - your hint makes things definitely better. :grin:
5. Yeah, I guess it was my better safe than sorry approach - especially strong since I've not yet reached unit testing in Rust on my learning path. Perhaps that should be the next thing on my list.
6. Ok, thanks for the pseudocode - I will play with the idea. As I mentioned to you while we were briefly chatting about this (was it on Friday?), I am also not exactly a fan of Rust shared pointers infecting my code, but that was all I could figure out to make it work for now. :sweat_smile:
7. And this one... well, it was a sahmeless copy&paste from some Rust GTK example and have not given it much thought :sweat_smile:
8. Yeah, they are - rand and png were initially used to generate some random art and save it to a file before I figured out how to paint stuff on a window, and now are dead leftovers. Will have them removed.
9. Will do.
10. Nice. Will play with the tool - thanks for pointing it out. :+1:

[szdanowi]

Current state:

1.

2. :heavy_check_mark: 1c88dcd8ef535c9f04a6a90a5f98203f75f74750
4. :heavy_check_mark: f6fd227349ba4784c943c6ea5a8c03ea2e4a58d8
5. :heavy_check_mark: fc4dcaf413319e91af75b9f0f3ec46b40ea44101
8. :heavy_check_mark: 2a6882111098de9becd43a85f2a404cef274d2e5

I will take care of the other points soon. :)

[szdanowi]

I gave a bit of thought to the point 7, and I think I'll leave it as it is now, for now at least - for the sake of a just slightly nicer call (&args().collect_vec() vs &args().collect::<Vec<_>>()) in an uninteresting place of code we'd be adding a crate dependency and an additional use statement, which seems a bit of an overkill to me. But I'll keep in mind that crate - possibly it's going to be useful sometime later. :)

1.

2. :heavy_check_mark: 1c88dcd8ef535c9f04a6a90a5f98203f75f74750
3. :heavy_check_mark: c38cd984da1cfcc6518d14251dfc4d02e587a7e0
4. :heavy_check_mark: f6fd227349ba4784c943c6ea5a8c03ea2e4a58d8
5. :heavy_check_mark: fc4dcaf413319e91af75b9f0f3ec46b40ea44101
7. :x:
8. :heavy_check_mark: 2a6882111098de9becd43a85f2a404cef274d2e5, 36b1815ef763104c8988dd5fbc2d5116eddc1d2c

[szdanowi]

1.

2. :heavy_check_mark: 1c88dcd8ef535c9f04a6a90a5f98203f75f74750
3. :heavy_check_mark: c38cd984da1cfcc6518d14251dfc4d02e587a7e0
4. :heavy_check_mark: f6fd227349ba4784c943c6ea5a8c03ea2e4a58d8
5. :heavy_check_mark: fc4dcaf413319e91af75b9f0f3ec46b40ea44101
7. :x:
8. :heavy_check_mark: 2a6882111098de9becd43a85f2a404cef274d2e5, 36b1815ef763104c8988dd5fbc2d5116eddc1d2c
9. :heavy_check_mark: 2a21b7503439df2182f980f17d1fdb07a8b79aa4
10. :heavy_check_mark: 93c8d225be383958a87af97810e7f313c63a2126

[szdanowi]

1. :heavy_check_mark: 021ede15e084e74dea50bbd29d2585da5701d88d
2. :heavy_check_mark: 1c88dcd8ef535c9f04a6a90a5f98203f75f74750
3. :heavy_check_mark: c38cd984da1cfcc6518d14251dfc4d02e587a7e0
4. :heavy_check_mark: f6fd227349ba4784c943c6ea5a8c03ea2e4a58d8
5. :heavy_check_mark: fc4dcaf413319e91af75b9f0f3ec46b40ea44101
7. :x:
8. :heavy_check_mark: 2a6882111098de9becd43a85f2a404cef274d2e5, 36b1815ef763104c8988dd5fbc2d5116eddc1d2c
9. :heavy_check_mark: 2a21b7503439df2182f980f17d1fdb07a8b79aa4
10. :heavy_check_mark: 93c8d225be383958a87af97810e7f313c63a2126

[szdanowi]

Regarding point 7 I've developed a proof of concept on a branch (see commit 08322be3e943ea65d1dc15543c50c444eb3c1b6e), although so far I've failed to entirely get rid of the need for reference counting of the model.

The model is needed in two event handlers now: one is the newly introduced events reactor, and the other is the functor that redraws the drawing area. I've tried to make redraw an another event, but then the reference to the cairo context would have to be passed as a part of the event, which fails miserably since the event seems to outlive the call to the drawing functor. I've also tried to pass a copy of the cairo context (as it is copyable), but no success there either as it pretty much results in drawing on a dev null - the application window never gets any drawing.

Any ideas @ArekPiekarz ?

Since the code on the branch is half done (it does not entirely get rid of the Rc-s as I hoped for) I haven't merged it to the master yet, but I tend to consider it a bit better anyway - even though the Rc-s are still there, all the actions (except for the redraw) are handled in a single central place, which is kind of nice, isn't it?

[ArekPiekarz]

The only potential solution I see from the documentation is manually invoking drawing instead of waiting for callback from connect_draw.

Requirements for painting: gtk::DrawingArea and cairo::Context. We already own the first one. We can get cairo::Context by using DrawingContext::get_cairo_context. DrawingContext would be retrieved from WindowExt::begin_draw_frame - it's a trait, I guess implemented by a window class. Note it requires enabling feature v3_22 in gdk in Cargo.toml. WindowExt::begin_draw_frame also requires cairo::Region parameter, which could be created with Region::create_rectangle. We would also need to call WindowExt::end_draw_frame.

Note that I haven't checked if it will actually work, so maybe keeping the Rc for model would be easier after all.

[szdanowi]

Well, I've gave it a try (see this commit: c77cbfeeca083b198a44b2fe936c58f846ae16b5), but it resulted, just as my yesterday's idea (here's a commit showing the idea: cabb3d04d0742e9ae972385c693f0227c4d77162), in screen flickering - the more elements to draw, the more severe flickering. Initially when reading the documentation for that WindowExt::begin_draw_frame method I thought that was what I was missing in my approach, but apparently not.

Somehow I'm under an impression, that the thread serving events is not the one that should fiddle with UI drawing, and we simply end up in a race condition where two threads try to draw the window - one GTK specific - that is meant to do just that, and the other that we're in when handling events.