Add a sort option popup to branch list view #2146
Conversation
|
Thanks for taking a go at this! If you look at other popups like the options or search-in-log popup you will notice that the way we do these is: using the navigation cursors to pick an option instead of adding a bunch of new key bindings for each option uniquely. if you could convert this on to the same that would be awesome |
UUGTech
force-pushed
the
feat-sort-branch
branch
from
74a2c02
to
dec6128
Compare
|
Thank you for pointing that out. Screen.Recording.2024-03-25.at.13.26.57.mov |
extrawurst
force-pushed
the
feat-sort-branch
branch
from
d329b95
to
d30b9ac
Compare
extrawurst
force-pushed
the
feat-sort-branch
branch
from
d30b9ac
to
0c1c973
Compare
src/popups/branchlist.rs
Outdated
| @@ -587,6 +706,15 @@ impl BranchListPopup { | |||
| .take(height) | |||
| .enumerate() | |||
| { | |||
| let date_local = Local | |||
| .timestamp_opt(displaybranch.top_commit_time, 0) | |||
| .unwrap(); | |||
There was a problem hiding this comment.
Would it be possible to change this from an unwrap to an unwrap_or to protect from panics just incase
There was a problem hiding this comment.
how is this even possible to pass CI?
checkout main.rs: #![deny(clippy::unwrap_used)]
There was a problem hiding this comment.
wtf MappedLocalTime: https://docs.rs/chrono/latest/chrono/offset/type.MappedLocalTime.html
lets do earliest and then unwrap the Option with unwrap_or_else
There was a problem hiding this comment.
I am also debating whether to store this DateTime<Local> instead inside of BranchInfo which should be fine as it is just twice as big as the i64 that we we do not have to do this mess everytime the screen renders
There was a problem hiding this comment.
but at the least the unwrap needs to go
| let date_text = displaybranch | ||
| .top_commit_time_local | ||
| .map_or("????-??-?? ".to_string(), |time| { | ||
| time.date_naive().to_string() + " " | ||
| }); |
There was a problem hiding this comment.
Sorry, I'm not sure how to deal with None case.
| pub top_commit_time: i64, | ||
| /// | ||
| pub top_commit_time_local: Option<DateTime<Local>>, |
There was a problem hiding this comment.
I added top_commit_time_local inside of BranchInfo.
But this could be None. I thought we should keep top_commit_time: i64 here. (top_commit_time is used for sorting) What do you think?
| // TODO: clean up | ||
| #[allow(clippy::too_many_lines)] |
There was a problem hiding this comment.
I don't have any idea to minimize this for now..
|
@extrawurst Sorry for the delay 🙇♂️ |
There was a problem hiding this comment.
when sorting it should also keep the selection: right now it only keeps the same index selected which will mostly be a different branch when sorting changes.
the popup list entries should also not use a focused (bold) style, make it more in line with other popups (search, fuzzy find)
|
@extrawurst
Easier way to find new branch is required in #2129, also I think sorting is that kind of feature.
Thank you. I'll fix this. |
yes please. the use case you describe is better fixed by providing quick scroll-to-top/bottom shortcuts like we also have in the commit log list view |
UUGTech
force-pushed
the
feat-sort-branch
branch
from
148d567
to
630828e
Compare
|
Other popups are in a bold style Which is better? Currently, branch_sort popup is in bold style. This is same with other popups. |
I made an issue for this. |
Let’s go non bold and make only the selection bold |
|
updated the style of these popups branch-sort popupScreen.Recording.2024-05-01.at.12.24.21.movlogs-search popupScreen.Recording.2024-05-01.at.12.25.00.movfuzzy-find popup
|
This Pull Request fixes/closes #2129, #2024
It changes the following:
I followed the checklist:
make checkwithout errorsScreen.Recording.2024-03-22.at.19.46.23.mov