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

Switch between multiple log views #3354

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mkock
Copy link
Sponsor

@mkock mkock commented Mar 2, 2024

  • PR Description

Seeing as the last activity related to this issue was over a year ago, I decided to take a stab at this.
The implementation should be fully backwards compatible. Simply add allBranchesLogCmdAlt1 and/or allBranchesLogCmdAlt2 to your config file to use them when cycling between log commands using 'a'.
You can even use allBranchesLogCmdAlt2 together with allBranchesLogCmd (skipping allBranchesLogCmdAlt1) if you want, it should not affect usability.

This is my first contribution to LazyGit, but I have experience with Go.

Changes:

  • Introduced two new optional user config commands, allBranchesLogCmdAlt1+2

  • When pressing 'a' in the Status view, cycle between non-empty, non-identical log commands

  • There will always be at least one command to run, since allBranhesLogCmd has a default

  • 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 (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@mark2185
Copy link
Collaborator

mark2185 commented Mar 2, 2024

Why constrain ourselves to two? Why not add allBranchesLogCmds (plural) instead and make it a []string?

It is backwards compatible, albeit maybe a bit odd to have both singular and plural, but we could easily modify the config to use the updated version.

@mkock
Copy link
Sponsor Author

mkock commented Mar 3, 2024

Why constrain ourselves to two? Why not add allBranchesLogCmds (plural) instead and make it a []string?

It is backwards compatible, albeit maybe a bit odd to have both singular and plural, but we could easily modify the config to use the updated version.

It's a good suggestion. And I guess that in the edge case where the user decides to have both allBranchesLogCmd and allBranchesLogCmds in their config, I could prepend the former to the latter, as a way to honor (rather than ignore) that the user has set a allBranchesLogCmd?

@mkock
Copy link
Sponsor Author

mkock commented Mar 3, 2024

Why constrain ourselves to two? Why not add allBranchesLogCmds (plural) instead and make it a []string?
It is backwards compatible, albeit maybe a bit odd to have both singular and plural, but we could easily modify the config to use the updated version.

It's a good suggestion. And I guess that in the edge case where the user decides to have both allBranchesLogCmd and allBranchesLogCmds in their config, I could prepend the former to the latter, as a way to honor (rather than ignore) that the user has set a allBranchesLogCmd?

@mark2185 I've updated the PR with your suggestion, please let me know if the change is okay.

t.Views().Status().
Focus().
Press(keys.Status.AllBranchesLogGraph)
t.Views().Main().Content(Contains("view1"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd maybe also add the check that it does not contain view2 at the same time. Just to be extra sure it's not displaying view1view2 somehow.

@mark2185
Copy link
Collaborator

mark2185 commented Mar 4, 2024

Looks good!

All that remains is updating the docs and cleaning up the history a bit:

  • rebasing on master would be much cleaner than merging master into the branch
  • squashing together the 1st and 3rd commit

@mark2185 mark2185 added the enhancement New feature or request label Mar 4, 2024
@mkock mkock force-pushed the switch-between-multiple-log-views branch 2 times, most recently from 5a472b0 to eeb4e6a Compare March 4, 2024 21:07
@mkock
Copy link
Sponsor Author

mkock commented Mar 4, 2024

Looks good!

All that remains is updating the docs and cleaning up the history a bit:

  • rebasing on master would be much cleaner than merging master into the branch
  • squashing together the 1st and 3rd commit

Apologies, I think I squashed the original commit that you commented on. I've updated the docs and improved the test case. Let me know if there is any other documentation that needs to be updated?

docs/Config.md Outdated
@@ -120,6 +120,7 @@ git:
fetchAll: true # Pass --all flag when running git fetch. Set to false to fetch only origin (or the current branch's upstream remote if there is one)
branchLogCmd: 'git log --graph --color=always --abbrev-commit --decorate --date=relative --pretty=medium {{branchName}} --'
allBranchesLogCmd: 'git log --graph --all --color=always --abbrev-commit --decorate --date=relative --pretty=medium'
allBranchesLogCmds: '' # a list of your favorite log commands (pressing 'a' in the status panel will cycle between them)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the default be []?

Copy link
Sponsor Author

Choose a reason for hiding this comment

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

@mark2185 Fixed. Thanks a bunch for the feedback thus far, it's appreciated 🙏🏻

@mark2185
Copy link
Collaborator

mark2185 commented Mar 5, 2024

Other this one minor nitpick, looks good to me! :)

@mark2185
Copy link
Collaborator

mark2185 commented Mar 5, 2024

All that's left is appeasing the CI and I think it's good to ship.

@mkock
Copy link
Sponsor Author

mkock commented Mar 7, 2024

@mark2185 Not sure what's up with the code coverage tool, but everything else is green now

@mark2185
Copy link
Collaborator

mark2185 commented Mar 7, 2024

Not sure, to be frank. Also you can squash all the commits now.

@stefanhaller I'm hoping you might have an ace up your sleeve regarding the code coverage.

@stefanhaller
Copy link
Collaborator

It's a known problem, this step currently always fails for PRs created from a fork.

- Introduced a new optional user config command, allBranchesLogCmds
- When pressing 'a' in the Status view, cycle between non-empty, non-identical log commands
- There will always be at least one command to run, since allBranhesLogCmd has a default
- Update documentation & write an integration test
- Update translation string
@mkock mkock force-pushed the switch-between-multiple-log-views branch from 0599049 to 41cefaf Compare March 7, 2024 19:45
@mkock
Copy link
Sponsor Author

mkock commented Mar 7, 2024

Done

@mark2185 mark2185 added the requires-maintainer-signoff PR is reviewed but requires final signoff label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requires-maintainer-signoff PR is reviewed but requires final signoff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch between multiple log views
3 participants