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

Settings updated event [cleanup] #8451

Merged
merged 1 commit into from Sep 14, 2020
Merged

Settings updated event [cleanup] #8451

merged 1 commit into from Sep 14, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 12, 2020

Continuation #8450

Proposed changes

  • Add the ability to receive an event about a change in the setting value.
  • Also, in the future, we can remove all classes inherited from Setting and use one generic.

Screenshots

After

image

We can replace

image

And use

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 3.4.3.9999
  • Build d4b0f48
  • Git 2.28.0.windows.1
  • Microsoft Windows NT 10.0.19041.0
  • .NET Framework 4.8.4220.0
  • DPI 96dpi (no scaling)

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

@ghost ghost self-assigned this Sep 12, 2020
@ghost
Copy link
Author

ghost commented Sep 12, 2020

@RussKie could you please do a review.

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #8451 into master will increase coverage by 0.12%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master    #8451      +/-   ##
==========================================
+ Coverage   53.00%   53.12%   +0.12%     
==========================================
  Files         877      880       +3     
  Lines       62129    62364     +235     
  Branches    11331    11348      +17     
==========================================
+ Hits        32930    33131     +201     
- Misses      26532    26565      +33     
- Partials     2667     2668       +1     
Flag Coverage Δ
#production 41.03% <53.33%> (+0.06%) ⬆️
#tests 94.31% <ø> (+0.04%) ⬆️

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

@RussKie
Copy link
Member

RussKie commented Sep 13, 2020

So if we get more properties that we can wire up events too we should consider INotifyPropertyChanged?

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.

👍

GitCommands/Settings/Setting.cs Outdated Show resolved Hide resolved
GitCommands/Settings/Setting.cs Outdated Show resolved Hide resolved
GitCommands/Settings/AppSettings.cs Outdated Show resolved Hide resolved
GitCommands/Settings/Setting.cs Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Sep 13, 2020

So if we get more properties that we can wire up events too we should consider INotifyPropertyChanged?

We still have one property "Value"
The rest are constant or depend on "Value"

GitCommands/Settings/Setting.cs Outdated Show resolved Hide resolved
@RussKie
Copy link
Member

RussKie commented Sep 13, 2020

So if we get more properties that we can wire up events too we should consider INotifyPropertyChanged?

We still have one property "Value"
The rest are constant or depend on "Value"

I'm thinking instead of

image

have something that works for every settings, e.g.:

AppSetitngs.SettingChanged += (s, e) => { ... }

@ghost
Copy link
Author

ghost commented Sep 13, 2020

We have different settings, this may require more changes.
We also need to know that this particular setting has been updated.
Better if it's in one place. At least for now.

@ghost
Copy link
Author

ghost commented Sep 13, 2020

image

@RussKie
Copy link
Member

RussKie commented Sep 13, 2020

Ugh... we'll have to deal with it one day :)

@RussKie
Copy link
Member

RussKie commented Sep 13, 2020

I had a go at refactoring it 3 years ago (ugh!) https://github.com/RussKie/gitextensions/tree/refactor_AppSettings

@ghost
Copy link
Author

ghost commented Sep 13, 2020

I had a go at refactoring it 3 years ago (ugh!) https://github.com/RussKie/gitextensions/tree/refactor_AppSettings

Are you planning to apply these changes?

@RussKie
Copy link
Member

RussKie commented Sep 13, 2020

I had a go at refactoring it 3 years ago (ugh!) https://github.com/RussKie/gitextensions/tree/refactor_AppSettings

Are you planning to apply these changes?

Maybe... One day.... When I have completed other things :) There are so many things I'd like to do, and there's so much time I have for this project.
Feel free to use it.

@ghost
Copy link
Author

ghost commented Sep 13, 2020

Renamed DefaultValue to Default

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

2 participants