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

Change ShowMergeCommits to HideMergeCommits #11069

Merged

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Jun 23, 2023

Follow up to #11065

Proposed changes

This better reflects the Git defaults and changes the only option ticked by default, that adds filter by selection.
Supposedly, this was a confusion in #11065

Change default for --simplify-merges to false to match defaults. Anyway, the value is not used by default as --full-history is false and --simplify-merges depends on --full-history.

--

This handles a setting with negation in the name, which can be confusing. The cange could be done in the user facing part only.
However, it is normally easier if the code and UI matches. This also remove the special handlig for the setting, negating by default.

Screenshots

Before

image

After

image

Test methodology

Tests updated

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned gerhardol Jun 23, 2023
@gerhardol gerhardol force-pushed the feature/i11065-no-merge-commits branch from 5d5e5ce to 6f8f6f7 Compare June 23, 2023 21:42
This better reflects the Git defaults and changes the only limiting
filter ticked by default.
Deselecting all options then is as Git defaults.

Change default for --simplify-merges to false to match defaults.
Anyway, the value is not used by default as --full-history is false and
--simplify-merges depends on --full-history.
@gerhardol gerhardol force-pushed the feature/i11065-no-merge-commits branch from 6f8f6f7 to 00f497f Compare June 23, 2023 21:43
@RussKie
Copy link
Member

RussKie commented Jun 24, 2023

-1 from me. I'm seeing this change as a value-remove not a value-add. The "negative" wording has higher cognitive load, e.g., you can't imaging an absence of something while you can imagine an existence of that. If you may remember the "Configuration" settings page featured negative wording which we inverted in #9372.

All other settings on this dialog use the "positive" wording:
image
With this change the user will have to go: "show A, show B, do not show C, show D, show E...".

. . .

As an OT, though related, perhaps we should align wording for some of the settings:

  • Show current branch only
  • Show reflog
  • Show only first parent
  • Show merge commits
  • Show full history
  • Simplify by decoration
  • Simplify merges

@RussKie
Copy link
Member

RussKie commented Jun 24, 2023

Perhaps, what we should do here is show a "Reset to defaults" button when the filter is changed (hidden by default). E.g., when this is happening:
image

This way the user will see that the filter is being applied. What do you think?

@gerhardol
Copy link
Member Author

The Git default and normally expected is reversed, so a negated interface here should be considered. No checkbox - no filter.

The drop-down menu for filter button already have a reset filter action.
Maybe the icon should be redesigned?
This is a separate (even if related) change for me.

@RussKie
Copy link
Member

RussKie commented Jun 24, 2023

The drop-down menu for filter button already have a reset filter action.

I think the confusion in #11065 was partly caused by the lack of visible information that the filter is set - a button to reset could help with awareness...

Maybe the icon should be redesigned?

No strong opinion (especially given that we lack in graphic design department).

The Git default and normally expected is reversed, so a negated interface here should be considered. No checkbox - no filter.

I can see that side of the argument too. I'm torn...
Git isn't know for being very user friendly or user oriented, it's designed by a geek for geeks. The whole purpose of the app is to democratise git and make it more approachable to an average person. So, an inversion of "negative" git settings is in our charter, and is something we've been doing.

@gerhardol
Copy link
Member Author

Slightly OT - also not this PR

We could stop considering Reflog a filter and remove it from Advanced filter
Maybe move to Commits in View menu. At least one change to reflogs in every release?

With that, Advanced filter (except Full history) is only limiting commits

image

@RussKie
Copy link
Member

RussKie commented Jun 28, 2023

Perhaps, what we should do here is show a "Reset to defaults" button when the filter is changed (hidden by default). E.g., when this is happening: image

This way the user will see that the filter is being applied. What do you think?

11069.mp4

What do you think?

@gerhardol
Copy link
Member Author

What do you think?

Since all settings are off by default, the need for an reset button is not big. I do not expect users to add many filters, so the main benefit is to indicate that a filter is active. The button is also an indicator, so OK for me.

The reset button does not work right now, no action.

Note: The video must be viewed fullscreen, as the controls hides the button now.

@RussKie
Copy link
Member

RussKie commented Jun 29, 2023 via email

@gerhardol
Copy link
Member Author

A notable fix - FilterInfo.HasFilter is currently inaccurate.

HasFilter works. It controls the filter button image.

Reflog is not a filter and full history and simlifymerges are really settings, but View menu is already full. Number of commits is a preference. (All i recall from top of my head, more settings.)

@gerhardol
Copy link
Member Author

gerhardol commented Jul 2, 2023

What do you think?

I would like to revert this commit.

For instance, the changed HasFilter() need to be replaced with a new IsDefault() to not set the modified icon when the user sets Current branch only.

Suggested to be moved to future PRs:

  • Remove refilter from Advanced filter?
  • Move Full history and Simplify merges to View menu, new group Path filter or to new page Settings-Detailed-Revision grid (move some other settings like Settings-General-Perfomance-LimitCommits.?
  • Simplify merges depends on Full history
  • Reset to default button in Advanced filter form?
  • Better button image at changes?

@RussKie
Copy link
Member

RussKie commented Jul 2, 2023 via email

@gerhardol
Copy link
Member Author

Reverted latest commit, kept RussKie reset button in https://github.com/gerhardol/gitextensions/tree/tmp/i11065-no-merge-commits
Opened "11095 for follow up
Suggest this is merged

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

I have no strong opinion on having everything as positive "Show xxx" or not - for filters.
"Everything unticked" as default gives a clear UX though.
The negation can be camouflaged by "Hide merge commits" - which I prefer over "No merge commits".
I like "Show only first parent" & "Show merge commits".

@gerhardol
Copy link
Member Author

The negation can be camouflaged by "Hide merge commits" - which I prefer over "No merge commits".

Change that

I like "Show only first parent" & "Show merge commits".

Changed to Show only first parent

Show full history

Not changing, this is specific to filtered paths (rewritten history), the option should be somewhere else.
(ended up here as there were no better place in menus, and Settings were considered hiding too much).

@gerhardol gerhardol changed the title Change ShowMergeCommits to NoMergeCommits Change ShowMergeCommits to HideMergeCommits Jul 6, 2023
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

LGTM, have not run

@RussKie
Copy link
Member

RussKie commented Jul 7, 2023

Reflog is not a filter and full history and simlifymerges are really settings, but View menu is already full. Number of commits is a preference. (All i recall from top of my head, more settings.)

Technically reflog may not be a filter, but from a user perspective it affects the list of displayed revisions, thus can be viewed as a "filter"-like setting. The same applies to other "Show Xyz" settings - those are filter-like too from the user perspective.

When I added the "reset" button (and changed the HasFilter condition to include the other settings) I was immediately visually alerted when I enabled reflog and other settings that affected the list of the displayed revisions, and I very much liked that.

@gerhardol
Copy link
Member Author

Reflog is not a filter and full history and simlifymerges are really settings, but View menu is already full. Number of commits is a preference. (All i recall from top of my head, more settings.)

Technically reflog may not be a filter, but from a user perspective it affects the list of displayed revisions, thus can be viewed as a "filter"-like setting. The same applies to other "Show Xyz" settings - those are filter-like too from the user perspective.

When I added the "reset" button (and changed the HasFilter condition to include the other settings) I was immediately visually alerted when I enabled reflog and other settings that affected the list of the displayed revisions, and I very much liked that.

But "Only current branch" is Git default and I do not want a notification when viewing all branches.
Similar, some prefer current-only and do not want a notification for current-only.
I do not want Reflog to be in my filter, the button show that.
Full-history and simply-merges is just a setting that applies for path filters, more a preference.

Reset filters and reset-to-default are separate actions.

@gerhardol gerhardol merged commit 8b5eab5 into gitextensions:master Jul 8, 2023
3 of 4 checks passed
@ghost ghost added this to the vNext milestone Jul 8, 2023
@gerhardol gerhardol deleted the feature/i11065-no-merge-commits branch July 8, 2023 18:19
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.

None yet

3 participants