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

Dashboard category context menu #5434

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Dashboard category context menu #5434

merged 1 commit into from
Oct 3, 2018

Conversation

NikolayXHD
Copy link
Member

@NikolayXHD NikolayXHD commented Sep 15, 2018

Fixes #4947
Fixes #4948

Depends on #5428

Screenshots

image

image

image

Tested on Windows 7

@RussKie
Copy link
Member

RussKie commented Sep 16, 2018

Please rebase

@NikolayXHD
Copy link
Member Author

rebase done

@RussKie
Copy link
Member

RussKie commented Sep 16, 2018

Works well, nice work.

Few things need further tuning though:

  • we should ask for a confirmation when deleting a category, because this is a destructive action and users may lose links to repositories, and those may not necessarily be in "recent repos" list
  • we should ask for a confirmation when clearing the "recent repos" list
  • whenever we rename or delete categories collapsed state of other groups must be retained
  • whenever we rename or delete categories the scroll position must be retained (ideally)

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@96b7ed6). Click here to learn what that means.
The diff coverage is 32.88%.

@@            Coverage Diff            @@
##             master    #5434   +/-   ##
=========================================
  Coverage          ?   35.28%           
=========================================
  Files             ?      606           
  Lines             ?    46222           
  Branches          ?     6304           
=========================================
  Hits              ?    16310           
  Misses            ?    29162           
  Partials          ?      750

@NikolayXHD
Copy link
Member Author

Few things need further tuning though:

we should ask for a confirmation when deleting a category, because this is a destructive action and users may lose links to repositories, and those may not necessarily be in "recent repos" list
we should ask for a confirmation when clearing the "recent repos" list
whenever we rename or delete categories collapsed state of other groups must be retained
whenever we rename or delete categories the scroll position must be retained (ideally)

Done.
Renamed category doesn't keep collapsed state, it allways becomes expanded.
It's not ideal, but acceptable in my opinion, because

  • Besides becoming expanded, the renamed category may naturally change its position in the list of groups because of alphabetical sort order. This way it will still mess other groups position in the screen even if collapsed state is preserved. Less frequently and with less amplitude, but it will.
  • I am lazy
  • I also noticed that keeping renamed category collapsed state was specifically excluded from requested improvements.

@RussKie
Copy link
Member

RussKie commented Sep 19, 2018

I am lazy
I also noticed that [...] was specifically excluded from requested improvements.

This made me laugh 👍

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 didn't realise this change would be so elaborate with p/invokes.
Well done, works well 👍

@RussKie
Copy link
Member

RussKie commented Sep 21, 2018 via email

@NikolayXHD
Copy link
Member Author

Personally I avoid shadowing existing members, if possible. Though it is not a hard and fast rule... Will it render the designer unusable?

Now that you asked, I don't see shadowing as such a great idea anymore, even if designer keeps working. Your suggestion on adding 2 methods InsertGroup and InsertGroups has a clear advantage of not adding any unnesessary complexity and potential for surprising side effects.

@RussKie
Copy link
Member

RussKie commented Oct 3, 2018

Awesome job! Thank you

@RussKie RussKie closed this Oct 3, 2018
@RussKie RussKie reopened this Oct 3, 2018
@RussKie RussKie merged commit 7aad0f6 into gitextensions:master Oct 3, 2018
@RussKie RussKie mentioned this pull request Oct 3, 2018
@NikolayXHD NikolayXHD deleted the dashboard_category_context_menu branch October 20, 2018 13:24
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