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

Sidepanel submodule improvements #8726

Merged
merged 8 commits into from Jan 21, 2021

Conversation

gerhardol
Copy link
Member

@gerhardol gerhardol commented Jan 6, 2021

Closes #8714
Second part of #8703
Based on #8703 that is set to automerge so not to be merged yet. Fully reviewable.

Best reviewed commit by commit

Proposed changes

  • Open submodule in left panel selects first/selected commit as in Submodule browse: Select both first/second commits #8703
  • Add actions in Commit/RevDiff worktree to submodules: Reset/Stash/Commit
  • Add Open in new instance for submodules
  • Tooltip with details for changed submodules. This adds extra git-diff for modules that are dirty only (for others the command were executed anyway). The number of changes reported are limited to five changes (similar to how the RevGrid tooltip is limited already)
  • For topmodule there is no submodule status message available, display the same tooltip as for the RevGrid WorkTree commit.
  • A few minor refactorings were required

Review commit by commit.
Several of the changes will be squashed at approval.

Screenshots

Using a testrepo with GE modules recursively added (originally to test performance)

Before After
image image
image image
(all submodules do not have tooltips) image
Commits selected when opening a submodule from left panel
image image

Test methodology

Manual
Mostly UI changes, using already existing


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

@ghost ghost assigned gerhardol Jan 6, 2021
@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #8726 (b38b46b) into master (668dafe) will decrease coverage by 0.05%.
The diff coverage is 31.97%.

@@            Coverage Diff             @@
##           master    #8726      +/-   ##
==========================================
- Coverage   56.05%   56.00%   -0.06%     
==========================================
  Files         902      903       +1     
  Lines       65134    65184      +50     
  Branches    11825    11844      +19     
==========================================
- Hits        36513    36507       -6     
- Misses      25702    25744      +42     
- Partials     2919     2933      +14     
Flag Coverage Δ
production 43.12% <31.97%> (-0.06%) ⬇️
tests 94.80% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Comment on lines 59 to 68
builder.Append(items.Count).Append(' ');

if (items.Count == 1)
{
builder.AppendLine(singular);
}
else
{
builder.Append(singular).AppendLine("s");
}
Copy link

Choose a reason for hiding this comment

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

var ending = string.Empty;

if (items.Count > 1)
{
    ending = "s";
}

builder.AppendFormat("{0} {1}{2}", items.Count, singular, ending)
    .AppendLine();

Copy link
Member Author

@gerhardol gerhardol Jan 7, 2021

Choose a reason for hiding this comment

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

If this is to be changed interpolated string should be used instead

                builder.Append($"{items.Count} {singular}{(items.Count == 1 ? "" : "s")}")
                    .AppendLine();

Copy link

Choose a reason for hiding this comment

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

A new string object will be created every time.

Copy link

Choose a reason for hiding this comment

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

image

@gerhardol
Copy link
Member Author

Rebased on master and removed draft, added review comments

@gerhardol gerhardol marked this pull request as ready for review January 7, 2021 11:39
@RussKie RussKie closed this Jan 9, 2021
@RussKie RussKie reopened this Jan 9, 2021
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

I checked out 4ba1413 but the submodule status didn't seem to work for me:

image

The context menu worked though:
image

GitCommands/Git/GitModule.cs Outdated Show resolved Hide resolved
GitUI/BranchTreePanel/RepoObjectsTree.Nodes.Submodules.cs Outdated Show resolved Hide resolved
GitUI/UserControls/RevisionGrid/ChangeCount.cs Outdated Show resolved Hide resolved
GitUI/UserControls/RevisionGrid/ChangeCount.cs Outdated Show resolved Hide resolved
@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 10, 2021
@gerhardol
Copy link
Member Author

I checked out 4ba1413 but the submodule status didn't seem to work for me:

Did you deactivate when we had performance problems a couple of years ago?

image

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 10, 2021
@gerhardol
Copy link
Member Author

Rebased after #8734, review comments

@RussKie
Copy link
Member

RussKie commented Jan 11, 2021

Did you deactivate when we had performance problems a couple of years ago?

image

Of course I did :)
image

I'm going to turn it on, and try again

@mstv mstv closed this in #8738 Jan 15, 2021
@gerhardol gerhardol reopened this Jan 15, 2021
@gerhardol
Copy link
Member Author

Still valid, #8738 fixed #8726 (comment)

@gerhardol
Copy link
Member Author

I'm going to turn it on, and try again

Any opinions?

I find that the tooltip to get the status is something I really missed, I do not have to open the submodules to see expected changes only (like dirty files).

@RussKie
Copy link
Member

RussKie commented Jan 16, 2021

I'm going to turn it on, and try again

Any opinions?

Yes, that worked :)

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.

Since I use submodules rarely, I cannot say much. I will review formally in the next days.

Should the two Open context menu items have more specific texts? Or do the different icons and tooltips suffice?

English.xlf needs an update (tooltips).

I guess #8703 was meant instead of #8700 in the PR description.

@gerhardol
Copy link
Member Author

Should the two Open context menu items have more specific texts? Or do the different icons and tooltips suffice?

The new instance could be called "Open with Git Extensions" as it is called in other menus, but I do not believe that is clearer.
Open in the current GE instance is a special case. This is how open of branches in the sidepanel works, so use the same instance is expected when double clicking.

/// <param name="path">path to the module</param>
private void SetModuleAsDirty(string path, bool force = false)
/// <param name="module">the submodule</param>
private void SetModuleAsDirty(GitModule module)
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@RussKie RussKie merged commit 24d0e89 into gitextensions:master Jan 21, 2021
@gerhardol gerhardol deleted the feature/browse-submodules-2 branch January 21, 2021 08:52
@RussKie RussKie added this to the 3.5 milestone Jan 26, 2021
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.

Reflects state of each submodule in the left panel
3 participants