Fix two issues with the "<-- YOU ARE HERE ---" label #2479
Conversation
Uffizzi Preview Environment
|
pkg/gui/presentation/commits.go
Outdated
| length int, | ||
| showGraph bool, | ||
| bisectInfo *git_commands.BisectInfo, | ||
| youAreHereStr string, // non-empty only when we are rebasing |
There was a problem hiding this comment.
I'm kind of abusing this parameter as both a way to pass in the translated string (Tr isn't known in here), and as a flag to say whether we should add the label or not. Hopefully the comment makes this clear enough.
pkg/gui/presentation/commits.go
Outdated
| fullDescription bool, | ||
| bisectStatus BisectStatus, | ||
| bisectInfo *git_commands.BisectInfo, | ||
| youAreHereStr string, // non-empty only if we're rebasing and this is the current commit |
There was a problem hiding this comment.
Same here, the string is both for knowing the translated string and a flag whether this particular commit should get the label. A separate bool might be clearer, but is also a bit redundant.
Alternatively, I considered passing in common.Common so that we know the translated text, and a bool. However, that would make it more awkward for the test below, which would now have to instantiate a common.Common.
jesseduffield
left a comment
jesseduffield
left a comment
There was a problem hiding this comment.
This is a welcome change: it was a hack to have something presentational on the commit itself. The plan with this presentation code is to switch from having standalone functions to having a struct which has fields like common.Common on it for i18n stuff. Given that fact, in this PR it would be good to have two args: one for common.Common and a boolean arg for whether we should show the 'you are here' label for a given commit.
You can use utils.NewDummyCommon() to instantiate the common.Common instance.
stefanhaller
force-pushed
the
fix-you-are-here-issues
branch
from
4549e27 to
1bce178
Compare
|
Ah, didn't know about |
|
Hm, testing this again I notice an ugly visual glitch: when hitting "e" on a commit, you see the YOU ARE HERE label briefly appear on the head commit before the commit list changes to the new state. It seems there's one extra refresh that displays the old model commits again, but the working copy is already in rebasing state. Do we need to store the working copy's rebasing state in the model so that it is consistent with the model's commits? |
|
Storing the working tree state in the model does seem to fix the glitch; please have a look at 483890d. |
pkg/gui/refresh.go
Outdated
| return err | ||
| } | ||
| gui.State.Model.Commits = updatedCommits | ||
| gui.State.Model.WorkingTreeState = gui.git.Status.WorkingTreeState() |
There was a problem hiding this comment.
If we're now setting this on the model, we should update the places that use gui.git.Status.WorkingTreeState() to now use gui.State.Model.WorkingTreeState, for the sake of consistency. But if we do that, I wonder how often that value would be stale. Perhaps never because it doesn't change without the list of commits also changing?
There was a problem hiding this comment.
Hm, I'm not sure I agree; the state in the model is really only there for making sure that the state matches the commits when drawing the commit list. For all other callers, I see no reason to use the model value; if we base behavior decisions on it (e.g. which options to show in the rebase menu), then that shouldn't depend on how long ago we last refreshed the commits list; it's better to use the most up to date value in these cases.
Maybe we could rename the model state to something like WorkingTreeStateCorrespondingToCommits to make that clearer.
There was a problem hiding this comment.
Makes sense. How about WorkingTreeStateAtLastCommitRefresh?
There was a problem hiding this comment.
I like that; force-pushed.
Instead, derive it from context at display time (if we're rebasing, it's the first non-todo commit). This fixes the problem that unfolding the current commit's files in the local commits panel would show junk in the frame's title. Along the way we make sure to only display the "<--- YOU ARE HERE" string in the local commits panel; previously it would show for the top commit of a branch or tag if mid-rebase.
This is the working tree state at the time the model commits were loaded. This avoids a visual glitch with the "You Are Here" label appearing at times when it is not supposed to.
stefanhaller
force-pushed
the
fix-you-are-here-issues
branch
from
483890d to
de3e483
Compare
|
Thanks for this @stefanhaller ! |
2 participants
This fixes two issues with the "<-- YOU ARE HERE ---" label:
go run scripts/cheatsheet/main.go generate)docs/Config.md) have been updated if necessary