Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
8301302: Platform preferences API #1014
8301302: Platform preferences API #1014
Changes from 26 commits
2d28c85
df03228
ba26271
00e9eb7
dbad68d
e16491a
2d1c7a9
0589078
f8a9c8a
40e2f28
b100c6c
4b9f75d
27022f3
02d6a2f
4172fa6
5081fda
08a3692
2837e33
9cfded0
c8fd0e7
1eaea48
c736dee
aae55f9
cb81762
a893fa4
8a5b213
7e826df
684f48b
167d4e8
ad74e39
38177a8
d012dec
9b607c6
6ec7e99
3cc29e9
9a817d1
bb6071c
a3d0010
17b2b08
9083d3e
9adef99
57c8d0c
bee1bae
89f1c8f
9cc3b4b
dbf24bf
0572a36
f291ea4
a04a8c6
0dff7a4
6c37544
db393fc
0759ddd
a2bd32d
070eab5
515395b
098ca17
eac1d82
efdab27
20cccc5
6457503
3354782
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this should be returned from a
private static final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. This is an overridden method, do you propose to introduce another
private static final
method, andgetPlatformKeyMappings
then calls out to this other method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that it is a constant, which I think would be good to extract. I think you may have mentioned it isn't called a whole lot, but recreating an immutable sometimes (large) map just seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only called once to initialize
PlatformPreferences
, so we should be fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be written more simply as
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look simpler to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class does not handle addition of keys in
current
- should we explain this fact in this method and/or the class javadoc?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't handle removals of keys in
current
, which is explained in the method documentation:"Returns a map that contains the new or changed mappings of current compared to old.
A value has changed if Objects#equals or Arrays#equals returns false when invoked with the old and new value."
If you think this is not clear enough, I can try to improve the wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it actually possible to have keys removed at runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set of preferences that is reported by a platform is hard-coded in the native platform implementation, and depends on the operating system version. It might indeed be the case that an OS upgrade/downgrade could result in a different set of reported properties, but can this happen while a JavaFX applications remains running?
In any case, even if that were to happen, the current behavior is that a preference that was once reported will not be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I spoke too soon. Mappings can actually be removed at runtime, one such example is
Windows.SPI.HighContrastColorScheme
, which is only available ifWindows.SPI.HighContrast
istrue
.This doesn't change the
ChangedValue
implementation, and there are tests that cover the key removal scenario.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that's the case, isn't it possible to have the same key added at runtime? this scenario cannot be handled by the current code, as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand what you mean that the same key can be added at runtime. The protocol is as follows:
PlatformPreferences.update
computes the difference between the currently known preferences, and the reported preferences. This is whatChangedValue.getEffectiveChanges
does. A mapping that is not included in the reported preferences doesn't mean that it was removed; if the native toolkit wants to signal removal, it needs to map the respective key tonull
instead.null
).This protocol gives native toolkits the option to choose how preferences are reported, some toolkits may always want to report all preferences, while other toolkits may only want to report the changed preferences. The burden of keeping track of the total set of preferences is placed on
PlatformPreferences
, not on the native toolkit. This allows us to implement the book-keeping logic in a single place, rather than in each toolkit individually.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for clarification. I misunderstood your earlier comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the code that handles added keys should be added here somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here.