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

Update FormRebase layout #10233

Merged
merged 7 commits into from Jan 21, 2023
Merged

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Oct 8, 2022

Contributes to #6183

Proposed changes

This may be a bit contentious, but I find the current layout of the rebase dialog offputting.
Notable changes:

  • Update the color scheme of the datagrid to make it less ugly
  • Show/hide buttons which are available for the current operation
  • Split the buttons into two categories
    • current commit specific - these live right under the datagrid
    • the whole operation specific - i.e., rebase/solve/continue/abort.

Screenshots

Before

  • Rebase
    image

  • Rebase with merge conflicts
    image

After

  • Rebase
    image

  • Rebase with merge conflicts
    image

Test methodology

  • Manual

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).
❕ The PR contains several commits to make it easier to review.


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

@ghost ghost assigned RussKie Oct 8, 2022
@pmiossec pmiossec changed the title Update DiffViewerSettingsPage layout Update FormRebase layout Oct 8, 2022
@pmiossec
Copy link
Member

pmiossec commented Oct 8, 2022

This may be a bit contentious, but I find the current layout of the rebase dialog offputting.

Same for me. I would like to redesign it since a long time and I finally started to work on it.... yesterday! 😜

There are really good things in what you did but there are things that would like to be changed:

  1. Use of 2 group boxes "general options" and "rebase onto" for the first line and the second line of settings
  2. More spaces for the "general options" groupbox to be able to add other options (the flow layout is no more suitable). I made a development to add the new "update-refs" option but the actual layout made that the new UI is not acceptable
  3. this form is doing 2 completely different things, selecting the rebase options and managing conflicts, so some things are strange in the 2 cases. When we are selecting the rebase options, the patchgrid takes a lot of spaces to display nothing. And when solving conflicts, the oprions are still clickable and even display misleading informations (see 'current branch') So, for me, we should either hide the not used part or make 2 different forms (what I started to do but will stopped until this PR is merged and we agreed on what to do). The 2 options will make more space for adding new "general options" and to display the patch grid.
  4. (Optional) If we have more space, I think that it's better to have the "There are unresolved merge conflicts" banner the top.
  5. Small thing that I have done also, put the branch label in bold so that the text is no more lost inside 2 long lines of text.

@RussKie RussKie added this to the vNext milestone Oct 8, 2022
@gerhardol
Copy link
Member

The proposal misses some useful buttons like Commit, skip currently applying.
Especially point 3 from pmiossec is something that is the core point, then it is easier to make more changes.

@pmiossec
Copy link
Member

pmiossec commented Oct 8, 2022

The proposal misses some useful buttons like Commit, skip currently applying.

@gerhardol For me this is good on this point. The buttons are below the patch grid and looking at the screen shots, I don't see lost of features...

@gerhardol
Copy link
Member

The proposal misses some useful buttons like Commit, skip currently applying.

@gerhardol For me this is good on this point. The buttons are below the patch grid and looking at the screen shots, I don't see lost of features...

Correct, missed that

@RussKie
Copy link
Member Author

RussKie commented Oct 10, 2022

There are really good things in what you did but there are things that would like to be changed:

  1. Use of 2 group boxes "general options" and "rebase onto" for the first line and the second line of settings
  2. More spaces for the "general options" groupbox to be able to add other options (the flow layout is no more suitable). I made a development to add the new "update-refs" option but the actual layout made that the new UI is not acceptable
  3. this form is doing 2 completely different things, selecting the rebase options and managing conflicts, so some things are strange in the 2 cases. When we are selecting the rebase options, the patchgrid takes a lot of spaces to display nothing. And when solving conflicts, the oprions are still clickable and even display misleading informations (see 'current branch') So, for me, we should either hide the not used part or make 2 different forms (what I started to do but will stopped until this PR is merged and we agreed on what to do). The 2 options will make more space for adding new "general options" and to display the patch grid.
  4. (Optional) If we have more space, I think that it's better to have the "There are unresolved merge conflicts" banner the top.
  5. Small thing that I have done also, put the branch label in bold so that the text is no more lost inside 2 long lines of text.

Nice suggestions (though I don't think I fully understand all of them, and may need some help to visualise those). Do you think we can do those change iteratively though? With my workload I can only do small spikes.

@pmiossec
Copy link
Member

There are really good things in what you did but there are things that would like to be changed:

  1. Use of 2 group boxes "general options" and "rebase onto" for the first line and the second line of settings
  2. More spaces for the "general options" groupbox to be able to add other options (the flow layout is no more suitable). I made a development to add the new "update-refs" option but the actual layout made that the new UI is not acceptable
  3. this form is doing 2 completely different things, selecting the rebase options and managing conflicts, so some things are strange in the 2 cases. When we are selecting the rebase options, the patchgrid takes a lot of spaces to display nothing. And when solving conflicts, the oprions are still clickable and even display misleading informations (see 'current branch') So, for me, we should either hide the not used part or make 2 different forms (what I started to do but will stopped until this PR is merged and we agreed on what to do). The 2 options will make more space for adding new "general options" and to display the patch grid.
  4. (Optional) If we have more space, I think that it's better to have the "There are unresolved merge conflicts" banner the top.
  5. Small thing that I have done also, put the branch label in bold so that the text is no more lost inside 2 long lines of text.

Nice suggestions (though I don't think I fully understand all of them, and may need some help to visualise those).

Do you think we can do those change iteratively though? With my workload I can only do small spikes.

Yes it should be possible even if it depends what we decide for 3.

  1. Could be made not too difficulty.
  2. I really need it (I think that 1. could help on it but not sure)
  3. Is the most challenging but need that we all agrees on what need to be done. Keep 1 screen, extract in 2 user controls and display them exclusively. Or cut current form in 2 distincts.
  4. could be done at the end, after 3.. Or never because it was a proposition...
  5. I already did it and could push it in your branch (or in my own to cherry pick)

Even if I don't have a lot of time also, starting from your branch, I could give a try to do 1.,2. and 5.

My main "problem" is that the "update-refs" option seems to be a great feature ( I still need to test it in details to be sure).
And if it has not a big impact, maybe it could have been interesting to include it in v4.0.
If I need to base the development on this PR, I'm dependent on the decision to include this one (which introduce much more changes) into v4.0
If I doesn't base on it, and do something to fix 2., the merge will be painful :(

About 1., I thought about it again and maybe a 3rd group box "Interactive rebase" maybe make sense containing "Interactive rebase " and "Autosquash"

@RussKie
Copy link
Member Author

RussKie commented Oct 10, 2022

I marked this PR for vNext because this dialog is complex, and I don't have an understanding of how it works. This is a very important and visible dialog, and I'd hate to include this into v4 release and have users broken (myself included). Having it in vNext will give us some time to confirm it's still working as expected.

And v4 should ship too.

@pmiossec
Copy link
Member

Ok. Thanks. I have not seen the milestone.
So no need to rush to get the new feature included....
It will be for me only in my personal build :D

@pmiossec
Copy link
Member

I give it a try in my branch (with same name)

Before starting the rebase:
image

When rebasing:

image

It solves nearly all the points (except the translation).
The patch grid could be hidden if needed in the first screen....

@gerhardol
Copy link
Member

The patch grid could be hidden if needed in the first screen....

Or expanded to (optionally) implement the interactive rebase to be able to provide commit information (what is changed etc)

@RussKie
Copy link
Member Author

RussKie commented Jan 21, 2023

Are there any objections to have this merged? If not, I'll merge this in the next few days.

@mstv
Copy link
Member

mstv commented Jan 21, 2023

No objections. (Currently, I am unable to test anything.)

Copy link
Member

@pmiossec pmiossec left a comment

Choose a reason for hiding this comment

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

Tested quickly and is working.
Nothing wrong stand out and as it is mainly Designer.cs changes and I'm not Designer.cs fluent 😉 , it seems good...

@RussKie
Copy link
Member Author

RussKie commented Jan 21, 2023

Thank you folks

@RussKie RussKie merged commit 109e240 into gitextensions:master Jan 21, 2023
@RussKie RussKie deleted the update_FormRebase_layout branch January 21, 2023 21:25
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

4 participants