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.

Sign up for GitHub

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

Jump to bottom

Browse Diff Reset: Multiple parents are not handled #4387

Closed
gerhardol opened this issue Jan 21, 2018 · 6 comments
Closed

Browse Diff Reset: Multiple parents are not handled #4387

gerhardol opened this issue Jan 21, 2018 · 6 comments
Labels
area: user experience type: bug 🐛 type: discussion
Milestone
3.00-beta1

Comments

@gerhardol
Copy link
Member

Do you want to request a feature or report a bug?
bug

What is the current behavior?
Browse Menu reset has incorrect handling for multiple parents (merge commits, as multi selection of more than 2 is ignored). The first parent is always used.

The label in the drop down menu is always describing the first parent too.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

  • Select merge commit
  • Select a file in the second group
  • The reset drop down menu contains correct "B" revision but the first "A" revision

What is the expected behavior?
The selected parent is used, not the first parent

Environment you encounter the issue:

  • GitExtensions version: all

Did this work in previous version of GitExtensions (which)?
no

image
@gerhardol
Copy link
Member Author

gerhardol commented Jan 21, 2018 •
edited

The selected revision (commit) is really global and could be visualized in some way common for all tabs.

There are a number of ways this can be solved:

  • Get the labels from the selected items to get the first/parents. The problem is what to do for multi selections where the items have different parents, just display ?
  • Use the parent "groups", the "Diff with: ". This will then have to be displayed also for single parents. The "selected" could always be displayed too, either in the diff list or in the menu. The dropdown would then just be Selected and Parent, no extra information.

Similar problem for the Difftool menu, but there the alternatives are displayed as A and B, so this display is correct (just confusing).

This could look like the following:
image

With two versions selected:
image

I would like to use this solution in other situations:

  • Provide better descriptions also for difftool (which is A and B? which did I select first and second?)
  • "Save as (B)" is confusing.
  • Remove the "dynamic" tab title (like "A: First --> B: Second") as it is confusing and is lost when selecting another tab
  • Display multi selections for diff tab (similar as for merge commits)

@gerhardol gerhardol changed the title Browse Diff Reset: Incorrect A Browse Diff Reset: Multiple parents are not handled Jan 21, 2018
@RussKie
Copy link
Member

RussKie commented Jan 22, 2018

I like the current presentation.
Perhaps we need to fix the parent issue

@gerhardol
Copy link
Member Author

I like the current presentation.

How to handle multi selections, "multiple" or something?

How to handle Difftool?

Do you want to keep the changing tab title?

@RussKie
Copy link
Member

RussKie commented Jan 22, 2018 •
edited

I meant this:
image
There is a lot of correlating information which IMO makes it easier to use.

From pure usability perspective I find the second lot of screenshots (with "Parent A, Current B") less meaningful.

How to handle multi selections, "multiple" or something?

Is that for multiple files selected in the diff list? If they are from the same parent - then parent, if they are from multiple parents - then yes, "multiple" may be justified.

Do you want to keep the changing tab title?

I personally find it annoying that the tab constantly changes its width. So I don't mind it we fix its title.

How to handle Difftool?

I am afraid I don't understand the question. Please elaborate.

@gerhardol
Copy link
Member Author

thanks for the feedback

If tab title is changed, there is no explanation of A and B
The "Save (B) as" could be renamed to "Save selected as"
For reset, you prefer the description if single A revision and for multi-selection

For difftool, the version is named as A and B as below, there we do not want the full description for every A and B (and A is not static as for reset).

image

The difftool could be presented as follows, where A presented as for Reset
("A" can be always be displayed in the filelist, but nothing there now that informs that it is A).

image

@gerhardol gerhardol mentioned this issue Jan 23, 2018
Merged
@gerhardol
Copy link
Member Author

Will open a separate issues for:

  • Browse Diff should allow comparison of multiple selected versions.
  • Remove the "dynamic" title for the Diff tab. Explain A and B in difftool dropdown (similar to reset)

Reset will always have Selected and Parent (First and second does not make sense with multiselection)
The "from"/first/parent commit will always be displayed (this is not a strict requirement, but it simplifies).

I will probably submit only one PR for all changes, as they are related. The PR will be separated in several commits tough. (More code is removed than was what added.)

Proposal of how this will look like in a few scenarios.

image

image

image

gerhardol added a commit to gerhardol/gitextensions that referenced this issue Feb 28, 2018
@gerhardol
RevisionDiff Update
09b8627 
Refresh of the FileStatusList used in various forms to more consistently use Revision and ParentGuid
Allow diff to many in diff Tab
Allow multiselect operations for Submodule operations

Fixes gitextensions#4485, fixes gitextensions#4484, fixes gitextensions#4396, fixes gitextensions#4387
TBD issue to multiselect revisions and see diffCommitSubmoduleChanges
Not implemented: "Switch selected revisions"
Not implemented: Replace assigning of FileStatusList.GitItemStatus with SetDiffs() for other modules (so Revision and Parent is consistently set)

Tests are are lame, they are just adding maintenance as they are not testing the capabilities of the actions.

WIP, may split this up slightly.
@gerhardol gerhardol mentioned this issue Mar 4, 2018
Closed
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 4, 2018
@gerhardol
FileStatusList handling of parents and selected revision
ce713ac 
Resolves gitextensions#4561
Refactoring to consistently use Revision and Parents in FileStatusList
FileStatusList forms should use that instead of assuming parents from RevisionGrid.GetSelectedRevisions().
The changes will remove code too.

Note: This commits enables correctrions for gitextensions#4387 and gitextensions#4396 etc too.
This was referenced Mar 4, 2018
Merged
Closed
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 4, 2018
@gerhardol
FileStatusList handling of parents and selected revision
503add8 
Resolves gitextensions#4561
Refactoring to consistently use Revision and Parents in FileStatusList
FileStatusList forms should use that instead of assuming parents from RevisionGrid.GetSelectedRevisions().
The changes will remove code too.

Note: This commits enables correctrions for gitextensions#4387 and gitextensions#4396 etc too.
@gerhardol gerhardol mentioned this issue Mar 4, 2018
Closed
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 4, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
dbcb13c 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
@gerhardol gerhardol mentioned this issue Mar 4, 2018
Merged
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 5, 2018
@gerhardol
FileStatusList handling of parents and selected revision
ab8d864 
Resolves gitextensions#4561
Refactoring to consistently use Revision and Parents in FileStatusList
FileStatusList forms should use that instead of assuming parents from RevisionGrid.GetSelectedRevisions().
The changes will remove code too.

Note: This commits enables corrections for gitextensions#4387 and gitextensions#4396 etc too.

Review comments: typecasting for GitRevision, consistently use GitItemsWithParents
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 6, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
acfced4 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 6, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
f45512a 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 7, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
48d37c0 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 10, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
f2d2424 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 10, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
9cd527e 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit to gerhardol/gitextensions that referenced this issue Mar 11, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation
a733416 
Fixes gitextensions#4564
Fixes gitextensions#4387
Fixes gitextensions#4396

Depends on gitextensions#4562 gitextensions#4663 gitextensions#4365 gitextensions#4366 gitextensions#4367 gitextensions#4368
Submitted to allow review of the outcome to gitextensions#4564
Note: Due to the structure of tests in a separate class, it is very difficult to split this issue in several commits

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (gitextensions#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes A and B revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff
gerhardol added a commit that referenced this issue Mar 18, 2018
@gerhardol
Browse Diff tab menu alternatives and presentation (#4569)
1dc1f81 
* Browse Diff: Show differences for several selections
* Browse Diff tab menu alternatives and presentation

Fixes #4564
Fixes #4387
Fixes #4396

Depends on #4562 #4663 #4365 #4366 #4367 #4368

Rewrote RevisionDiffController to make it possible to have some kind of tests. The tests are retrofitted to the current functionality as they are just testing the menu items and not the actions themselves. (The tests adds may more maintenance than they give benefits but the formal test coverage increases.)

Browse Diff:
Submodules actions available for multi select (#4568)
Presents the Reset options for the correct parent in multi select situations
No longer differ between parent-child and first-second for Reset
Difftool: Describes Selected (B) and First (A) revisions in the menu
Difftool arguments depends on parent availability
Better detection of parents to A/B for diffs

Commit:
Limit DiffTool and FileHistory to tracked

FormDiff:
Limit FileHistory to Tracked
Use RevisionDiffController to align to Browse-Diff

* Extract common revision diff context menu logic into IRevisionDiffContextMenuController
@RussKie RussKie added this to the 3.00 milestone Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: user experience type: bug 🐛 type: discussion
Projects
None yet
Milestone
3.00-beta1
Development

No branches or pull requests
2 participants
@RussKie @gerhardol