Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SelectedCommit and SelectedPath Generic for all views #3752

Closed
wants to merge 3 commits into from

Conversation

abelmul
Copy link

@abelmul abelmul commented Jul 15, 2024

Fixes #3663

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

Copy link
Collaborator

@stefanhaller stefanhaller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for attempting to fix this! It needs a bit of work though, see below. 😄

if commit == nil {
commit = commitShimFromModelCommit(self.c.Contexts().SubCommits.GetSelected())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic doesn't make sense: we need to test which panel is focused, and set commit to LocalCommits.GetSelected() if it's the local commits panel, or to ReflogCommits.GetSelected() if it's the reflogs panel, or to SubCommits.GetSelected() if it's the subcommits panel. Or to nil if it's not any of these, although we might consider setting it to LocalCommits.GetSelected() in that case because that's probably a useful default.

I think you can use self.c.CurrentSideContext() to find out which panel is focused.

SelectedReflogCommit: commitShimFromModelCommit(self.c.Contexts().ReflogCommits.GetSelected()),
SelectedLocalCommit: commit,
SelectedReflogCommit: commit,
SelectedCommit: commit,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is backwards. We need to keep setting SelectedLocalCommit, SelectedReflogCommit, and SelectedSubCommit to what they were set to before, for backwards compatibility. Only SelectedCommit will point to one of them, depending on context; see above.


if path == "" {
path = self.c.Contexts().CommitFiles.GetSelectedPath()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here; testing if path is empty doesn't make sense. We need to set SelectedPath to either Files.GetSelectedPath() or CommitFiles.GetSelectedPath(), depending on which view is focused.

@stefanhaller
Copy link
Collaborator

Since I didn't hear back, I opened a PR to fix this properly (#3809), so I'll close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectedSubCommit is nil in the commits tab
2 participants