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

Hide CommitInfo panel for virtual commits #4096

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Hide CommitInfo panel for virtual commits #4096

merged 1 commit into from
Nov 8, 2017

Conversation

gerhardol
Copy link
Member

It has no information for staged/unstaged, just blank
It should later be used for commit without a popup as discussed in #4031 etc

Discussed in #4031

Changes proposed in this pull request:

  • Make sure the commit info panel is collapsed for artificial commits

Screenshots before and after (if PR changes UI):

  • Only before - the blank window is removed for artificial commits with this PR
    image

How did I test this code:

  • Manual

Has been tested on (remove any that don't apply):

  • Windows 10

@RussKie RussKie mentioned this pull request Oct 25, 2017
Closed
@RussKie
Copy link
Member

RussKie commented Oct 25, 2017

How about hiding the commit info tab?

@gerhardol
Copy link
Member Author

The tab is already hidden. If not in current master, then in other PRs.

@gerhardol
Copy link
Member Author

Confirming that Commit info tab is already hidden.
The FileTree is visible in master though.
(There is an attempt to remove the tab but that is not properly implemented.)
Corrected in #4084 (as a lot of other issues that are hard to separate). This specific change I guess I can separate though.

@gerhardol gerhardol mentioned this pull request Oct 25, 2017
Merged
@RussKie
Copy link
Member

RussKie commented Oct 26, 2017

I'm getting confused with so many open related PRs... 🤕

Further to my comment #4084 (comment). I definitely getting a noticable lag navigating from a normal commit to an artificial one. Looking at the log, it takes about 800-900ms (combined) to collect data for the artificial commit and 600-700ms for the normal one.
The lag seems to be happening at the UI layer before git commands are fired. So there must be about extra 1-1.5 seconds of inefficiency buried in the code...

image

@gerhardol
Copy link
Member Author

I am confused by submitting the PRs...
It is hard to find what is submitted and not too. (I cannot even commit the major thing I want to change, diff artificial to normal commits while all other changes are pending.)

For the delay, I do not see any extra delay normal->artificial caused by this PR or #4084.
Any hints how to reproduce?
You have certainly much longer update times than I can reproduce. What is your system setup?

The handling can certainly be improved, for instance no point to ask for changed files if FileSystemWatcher has not reported any changes.
I plan to look into the efficiency after this block of PRs are handled. The solution will add to the complexity though.

@RussKie
Copy link
Member

RussKie commented Oct 26, 2017 via email

@RussKie
Copy link
Member

RussKie commented Oct 26, 2017 via email

@RussKie
Copy link
Member

RussKie commented Oct 26, 2017

I've run it on my desktop and I do not get any significant performance issues. Everything is in order of magnitude faster than on my work laptop

@gerhardol
Copy link
Member Author

Sidenote:
Git was slow like *** at work some time ago. A git status could take 10s (smaller repos but many submodules).
The reason was that the Symantec Virus was executing checks for every Git file operation. There were some kind of exception for Git applied due to that I believe. (Antivirus program create more problems than they solve i my opinion.)

But do you still believe that there is a performance problem introduced by this or any related PRs?
If not, can we leave the performance issues that exist to #4069?
(I plan to update the proposal some, to improve also the normal->artificial scenario.)

@gerhardol
Copy link
Member Author

I could update the patch similar to below, to handle "restart required"
(except that the variable should be bool (not bool?) and set in Init

0001-tmp-prposal.patch.txt

@RussKie
Copy link
Member

RussKie commented Oct 26, 2017 •
edited
Loading

Yes, AV is a constant headache for devs at work... and security people aren't very engaging
Let's park the performance for now and concentrate on the UI

What do you think about the following:

  • if a user attempts to change the value of "Show the commit info on the right" - either check or uncheck - we ask the user if they wish to apply changes and restart
  • if the users agrees - save changes and restart the app
  • if not - reset the value to what it was

@jbialobr - thoughts?

PS: do it as a separate piece of work though, if this is something we want to pursue

[edit]
PS2: how do we handle if multiple instance of GE open? do we kill or restart all of them?

@vbjay
Copy link
Contributor

vbjay commented Oct 27, 2017 via email

@jbialobr
Copy link
Member

I am sorry for a late reply, but recently I don't have much free time. Generally I don't like the idea of disappearing tabs and panels that change the layout of other controls. If you hide that panel the revisions grid will resize causing to many visual changes. As for hiding tabs, if you hide one and then go to a commit that causes showing it again then if the hidden tab was originally selected then after showing it the selection should be restored. Otherwise it is a bad UX.

@jbialobr
Copy link
Member

if a user attempts to change the value of "Show the commit info on the right" - either check or uncheck - we ask the user if they wish to apply changes and restart

If you want to put some effort here, it is better to make it not requiring a restart.

@RussKie
Copy link
Member

RussKie commented Oct 27, 2017

that's an option too :)

@RussKie
Copy link
Member

RussKie commented Oct 27, 2017 via email

@RussKie
Copy link
Member

RussKie commented Oct 27, 2017 via email

@gerhardol
Copy link
Member Author

The setting is now dynamic
(not the most rewarding hours of my life).

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Oct 27, 2017
@gerhardol
Merge remote-tracking branch 'origine/master' into feature-4084/n4031…
a6cc232 
…-formbrowse-diffmenu-artificial-submodule

This basically drops the changes in GitUI/CommandsDialogs/FormBrowse.cs (and Designer)
One minor part handled in gitextensions#4096
Other changes will be handled in a new PR
@RussKie
Copy link
Member

RussKie commented Nov 5, 2017

Works nicely, even on Linux, good job.

Two things though:

  1. I suppose it was an initial oversight - the setting "Show revision details next to the revision list" should not be available under Mono, as the panels are not resizable and a lot of information gets clipped off
    image

  2. The label should be reworded to remove "requires restart" bit

@gerhardol
Copy link
Member Author

  1. Mono
    Can we take that as a separate issue?
    Or even ignore if Mono is dropped in v3?

  2. UI changes
    While I have submitted changes to change translations, the right way to go is to sync with Transifex. The latest in Transifex should be pulled first, then changes done to translations, then pushing. Requires admin rights in Transifex (@ jbialobr, @KindDragon , @spdr870 ).

RussKie
RussKie reviewed Nov 7, 2017
View reviewed changes
GitUI/CommandsDialogs/FormBrowse.cs Outdated
@@ -1073,25 +1082,44 @@ internal enum UpdateTargets
private UpdateTargets _selectedRevisionUpdatedTargets = UpdateTargets.None;
private void RevisionGridSelectionChanged(object sender, EventArgs e)
{
if(_ShowRevisionInfoNextToRevisionGrid != AppSettings.ShowRevisionInfoNextToRevisionGrid.ValueOrDefault)
{
_ShowRevisionInfoNextToRevisionGrid = AppSettings.ShowRevisionInfoNextToRevisionGrid.ValueOrDefault;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't all other references to AppSettings.ShowRevisionInfoNextToRevisionGrid.ValueOrDefault be replaced with _ShowRevisionInfoNextToRevisionGrid ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This checks if the setup has changed since last time and refreshes the layout if so. It may be possible to always refresh the layout, but then the position etc should be remembered.

Copy link
Member

Choose a reason for hiding this comment

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

my questions was about use of AppSettings.ShowRevisionInfoNextToRevisionGrid.ValueOrDefault in the code. my understanding that all references should be replaced with _ShowRevisionInfoNextToRevisionGrid (i.e. gerhardol/gitextensions@4ca3e3c)

@RussKie
Copy link
Member

RussKie commented Nov 7, 2017

@gerhardol please have a look gerhardol@4ca3e3c

Essentially I just squashed your PR (made it easier to review) and tweak the setting to work for Windows only.

@gerhardol
Copy link
Member Author

@RussKie I do not see the commit when cloning. What have I missed in Git setup?

I have therefore only viewed the changes, seem OK to me and should be mergable, but I would be more confident if I could compare to what I tested myself.

@RussKie
Copy link
Member

RussKie commented Nov 7, 2017 via email

@gerhardol
Copy link
Member Author

gerhardol commented Nov 8, 2017 via email

@jbialobr
Copy link
Member

jbialobr commented Nov 8, 2017

I couldn't send a pr to your fork because of gh's interface

In such cases GitExtensions GitHub plugin comes with a rescue. I was not able to send a PR to your fork using GH web page, but GitExtensions had no problem with that.

@gerhardol
Copy link
Member Author

gerhardol commented Nov 8, 2017 via email

@RussKie
Copy link
Member

RussKie commented Nov 8, 2017

Still AppSettings though

???

@RussKie RussKie force-pushed the bugfix/n4031-hide-commitpanel-artifical-commits branch from 7895210 to 4ca3e3c Compare November 8, 2017 21:23
@RussKie
Hide CommitInfo panel for virtual commits
3c4d937 
* Hide CommitInfo panel for virtual commits - it has no information for staged/unstaged, just blank
* Make setting for "Show commit next to revision" dynamic (not requiring a restart)
@RussKie RussKie force-pushed the bugfix/n4031-hide-commitpanel-artifical-commits branch from 4ca3e3c to 3c4d937 Compare November 8, 2017 21:26
@RussKie RussKie merged commit fbef847 into gitextensions:master Nov 8, 2017
@RussKie RussKie added this to the 2.51 milestone Nov 8, 2017
@gerhardol
Copy link
Member Author

Just to finish the discussion (after a long day at work):
The internal variable _showRevisionInfoNextToRevisionGrid is synced to the settings in RevisionGridSelectionChanged(). AppSettings could be used everywhere else I believe.
It is possible to remove _showRevisionInfoNextToRevisionGrid, but then LayoutRevisionInfo() runs more often and the splitter position must be remembered. Right now the size stays until the setting is changed or program is restarted.

But setting the same value that already existdoes not seem to cause "flickering for me at least.

@gerhardol gerhardol deleted the bugfix/n4031-hide-commitpanel-artifical-commits branch November 19, 2017 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.51.RC1
Development

Successfully merging this pull request may close these issues.
4 participants
@gerhardol @RussKie @vbjay @jbialobr