gitk: combine author and date columns - #46
Conversation
239a0ed to
8ad95ee
Compare
|
Thank you. Generally, I think that this change has merit. Without having looked at the code, yet, I have some questions. The commit message of 9eb5975 says:
Both sentences describe the solution of a problem each, but not the problems. Please extend the commit message with problem descriptions. Furthermore:
"Preserve a ratio" and then also "restore a sash position"? Which one is it? Or do we need both and why is that? Or am I misunderstanding the text? Commit 8ad95ee says:
It is important that both the old and the new setting are saved in different variable names and that the old setting is still saved and restored even if it is not used, because people (like me) want to switch between the old and the new layout without having to rearrange columns every time. The old setting can be removed after everyone has settled with the new feature (that would be some time in the future). You mention that you have been helped by AI. Since this code is sent downstream to the Git repository, their AI rules must be obeyed. Equally important is that I want to be sure that you have checked and carefully reviewed and understood everything the AI produced. I am not going to look at the code until there is sufficient insurance that you did. (Blatantly put, I don't want to review code produced by someone else with AI.) |
Gitk uses three columns for commit message, author, and time. After the window is resized, users often need to adjust two sliders repeatedly to make the columns fit the available space. Combine author and time in one metadata pane so only one slider is needed. Keep time right-aligned, truncate author when necessary, and preserve the pane ratio when the window width changes. Use a 60:40 default split in favor of commit messages. Store the new ratio separately and preserve the legacy two sash settings so users can switch between old and new layouts without rearranging the columns. Clip search highlights to the visible author text, and relayout only visible rows to keep resizing responsive. Signed-off-by: Teddy Zhang <lostindark@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6389903b-f004-43ef-ae89-cc56ac4281e6
8ad95ee to
2f71b64
Compare
Updated PR message and hope that clear things a bit. I've also modified the code to preserve the old setting. However, the old behavior is not preserved in updated code (user can't toggle a setting to get back the old behavior). Is that what you're asking for? That will needs way more code to be able to support both behavior. Regarding the AI code, yes I manually reviewed all the code generated and ensure I understand the logic and the code is doing what I expected. |
|
Thank you for the update.
Just to confirm: I was not asking for that, but only to preserve the old layout settings for the case when an old version of Gitk is used that does not have the new layout. |
There was a problem hiding this comment.
The commit could reasonably be split into multiple steps.
- Remove
canv3, just append the date to the author. (The result looks awful, but it's just a step to the final result.) Serialize geometry without compatibility concerns. - Shorten author text to fit the text into the window.
- Right-align the date.
- Make selection of found, but hidden, text work.
- Resurrect compatibility geometry serialization. Having this in its own step would make it simple to back it out again later by just reverting this commit.
I may be wrong about the simplicity of the first step 1 due to how the linedtags have to be treated. It would also be possible to merge step 3 into step 1 (if that is simpler) with overlapping author and date texts, which step 2 would then rectify.
| .tf.histframe.pwclist sashpos 1 [lindex $::geometry(pwsash1) 0] | ||
| .tf.histframe.pwclist sashpos 0 [lindex $::geometry(pwsash0) 0] | ||
| resizeclistpanes .tf.histframe.pwclist \ | ||
| [winfo width .tf.histframe.pwclist] |
There was a problem hiding this comment.
We should use %W instead of the full window name in the bound script. (Then this doesn't have to be line-wrapped, either.) Note that we can't use %w, because it is not defined for the <Map> event.
| } | ||
| if {$sash0 > $w - 20} { | ||
| set sash0 [expr {$w - 20}] | ||
| } |
There was a problem hiding this comment.
This order of boundary checks favors commit authors+dates over commit messages. Could we not swap the two checks "if sash < 30" and "if sash > w-20", so that for extremely narrow windows we get to see the commit message rather than the author and date?
| set high [expr {$mid - 1}] | ||
| } | ||
| } | ||
| return [list "[string range $text 0 [expr {$low - 1}]]$suffix" $low] |
There was a problem hiding this comment.
This implementation of the bisection feels unnatural, because there are so many off-by-1 applied. In particular, that the result must be corrected by -1 is dubious. I'm not saying that it is wrong, only that there must be a way to have simpler logic that does not need these corrections all over the place.
| } | ||
|
|
||
| proc layout_author_date {id width} { | ||
| global canv2 linentag linedtag commitinfo authorchars |
There was a problem hiding this comment.
I have a hard time understanding what the "n" and the "d" in linentag and linedtag are.
Never mind. These aren't concepts that are new in this commit.
|
|
||
| # mark the bits of a headline or author that match a find string | ||
| proc markmatches {canv l str tag matches font row} { | ||
| proc markmatches {canv l str tag matches font row {visiblechars {}}} { |
There was a problem hiding this comment.
It may be worthwhile to mention in the function comment how visiblechars affects the operation.
2 participants
Problem
Currently gitk uses 3 columns to show commit message/author/time. When window resize, those columns width often get reset, and user need to adjust them multi time to make them to fit screen better.
Solution
The goal of this change to simplify user operation by combine author and time into 1, so user only need to drag one slider. Also the ratio of the width is remembered, so user doesn't need to change column width most of the time.
Change Summary
Before (default could look really bad):
User needs to drag 2 slider to make it look better:
After (note Author and Commit Time are in the same column now):
Resized
Testing