Added direction.rs

[marianomarciello]

Added Direction enum:

• North;
• East;
• South;
• West.

This enum is used to search the neighbor Rect in a given direction.

Added functions:

• find_north: find the north neighbor;
• find_east: find the east neighbor;
• find_south: find the south neighbor;
• find_west: find the west neighbor;
• find_neighbor: wrapper for the above functions.

This commit is related to the leftwm issue "Directional Focus and Window movement (771)".

[VuiMuich]

First of all, thanks for your contribution and very nice you added tests.

A few thoughts:

• just a nit, but imo in the doc comments the little arrows ar a easy to overlook, maybe "indent" them one space from the outline?
• I believe the let current_rect at the beginning of each find_{north,east,south,west} body should instead of .or(None).unwrap() either return None early or at least .expect("No current rect found.") or something
• can we find something nicer instead the nested for loops? Either put it in a generalized function, that all the find_{north,east,south,west} functions call, or rewrite them as iterators, idk..
• It would be kind of nice, if the last return would be our actual result and not the fallback for "no result in the loops"
• the checks if current is the north-most or west-most are inconsistent (== vs <=), is this for a reason I am missing?

Thats all I could catch so far.

[marianomarciello]

Hi, thanks a lot for your time, I will respond in line

First of all, thanks for your contribution and very nice you added tests.

A few thoughts:

• just a nit, but imo in the doc comments the little arrows ar a easy to overlook, maybe "indent" them one space from the outline?

This should be fixed with this commit

• I believe the let current_rect at the beginning of each find_{north,east,south,west} body should instead of .or(None).unwrap() either return None early or at least .expect("No current rect found.") or something

This should be fixed with this commit

• can we find something nicer instead the nested for loops? Either put it in a generalized function, that all the find_{north,east,south,west} functions call, or rewrite them as iterators, idk..

Also fixed in this commit

• It would be kind of nice, if the last return would be our actual result and not the fallback for "no result in the loops"

Now the return is the return value of a function call.

• the checks if current is the north-most or west-most are inconsistent (== vs <=), is this for a reason I am missing?

Now the check is done using <= or >=, that's because maybe a floating windows can exceed the display size. I don't know if this is a possible scenario, but I like most to use <= and >= rather than using == just to be on the safe side.

Thats all I could catch so far.

Thanks again. Let me know if you see further improvement 😄

[VuiMuich]

Thanks for taking your time to add these changes.

Indeed there are some things I believe we could improve a little here:

My idea for the doc comments were more like this:

  ///    North
    ///
    /// +---------+
    /// | ^       |
    /// |         |
    /// |         |
    /// +---------+
  ///    East
    ///
    /// +---------+
    /// |       > |
    /// |         |
    /// |         |
    /// +---------+
  ///    South
    ///
    /// +---------+
    /// |         |
    /// |         |
    /// | v       |
    /// +---------+
  ///    West
    ///
    /// +---------+
    /// | <       |
    /// |         |
    /// |         |
    /// +---------+

As in my head it is "find neighbours to this edge"

I would prefer the utility function search_loops further down, I am indecisive though if it should be before or after the impl Directions.

Also renaming a couple of symbols would be nice:

• the search_loops is ok-ish but it only describes what it does technically not what the work is its doing
• the first_loop and second_loop ar ok inside the utility function, but again it would be nice to be more descriptive about the purpose of the loop
• in the find_{n,e,s,w} functions it is important to be more descriptive about what these first_loop and second_loop variables are holding not where they get fed into

Otherwise this looks arleady a lot cleaner and better readible.

[marianomarciello]

Hi, thanks again for your review and for your time. 😄

[VuiMuich]

This one single change to make it plural for consistency.
Otherwise lgtm.

[hertg]

Code looks good overall, thank you very much!
I commented on the implementation of search_nearest_neighbor as I'm confident that we could make that code a bit more efficient.

[mautamu]

Thank you so much for doing this PR, @marianomarciello! Apologies that it has fallen between the cracks. This appears to be good to me so I will go ahead and merge it tomorrow around 0400Z provided there are no requested changes.

Best,
Mau

[marianomarciello]

Thank you so much for doing this PR, @marianomarciello! Apologies that it has fallen between the cracks. This appears to be good to me so I will go ahead and merge it tomorrow around 0400Z provided there are no requested changes.

Best, Mau

Hi @mautamu thanks for your time :)

[hertg]

I'm good with this being merged. I apologize for keeping you hanging @marianomarciello, this somehow wasn't on my radar anymore. Great work, and thank you so much for your contribution :) ❤️

[hertg]

I published this as 0.9.0