Modify

Opened 8 weeks ago

Closed 4 weeks ago

Last modified 2 weeks ago

#19196 closed enhancement (fixed)

[PATCH] Don't require a restart when a MapPaint color is changed

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 20.06
Component: Core mappaint Version:
Keywords: Cc:

Description

All this patch does is call MapPaintStyles.readFromPreferences().

Attachments (4)

19196.patch (1.4 KB) - added by taylor.smock 8 weeks ago.
19196.1.patch (1.6 KB) - added by taylor.smock 5 weeks ago.
Use a listener to get preference change events
19196.2.patch (3.0 KB) - added by taylor.smock 5 weeks ago.
Fix an issue where the preference listener could be called twice in the same preference change invocation
19196.3.patch (1.3 KB) - added by taylor.smock 4 weeks ago.
Fix unit tests

Download all attachments as: .zip

Change History (13)

Changed 8 weeks ago by taylor.smock

Attachment: 19196.patch added

comment:1 Changed 5 weeks ago by taylor.smock

Is there any interest for this patch?

comment:2 Changed 5 weeks ago by stoecker

I don't think that patch is not right. It only cares for mappaint. There are other places in JOSM which will not switch color without restart. Thought the line may be additional helpful to reach the goal in most cases, which is probably enough.

OTOH I think this breaks the component separation efforts.

comment:3 in reply to:  2 ; Changed 5 weeks ago by taylor.smock

Replying to stoecker:

I don't think that patch is not right. It only cares for mappaint. There are other places in JOSM which will not switch color without restart. Thought the line may be additional helpful to reach the goal in most cases, which is probably enough.

That is somewhat surprising. The current code only returns true if a mappaint color is changed ( https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/preferences/display/ColorPreference.java#L392 ). Specifically,

if (e.info.getValue() != null && e.toProperty().put(e.info.getValue())
    && NamedColorProperty.COLOR_CATEGORY_MAPPAINT.equals(e.info.getCategory()))

is the only place where the ret value is set to true, which means that only map paint colors require a restart (as far as the current code is concerned). I did some spot checking with other colors (non mappaint), and they worked without a restart.

OTOH I think this breaks the component separation efforts.

The only other thing I can think of is to add a preference change listener to Mappaint.

I presume you were talking about #15229?

comment:4 in reply to:  3 Changed 5 weeks ago by stoecker

Replying to taylor.smock:

is the only place where the ret value is set to true, which means that only map paint colors require a restart (as far as the current code is concerned). I did some spot checking with other colors (non mappaint), and they worked without a restart.

Ah. So maybe this was improved already.

OTOH I think this breaks the component separation efforts.

The only other thing I can think of is to add a preference change listener to Mappaint.

I presume you were talking about #15229?

Yes.

Changed 5 weeks ago by taylor.smock

Attachment: 19196.1.patch added

Use a listener to get preference change events

comment:5 Changed 5 weeks ago by taylor.smock

I've added a preference change listener in MapPaintStyles, and the if statement in ColorPreference has been modified accordingly.

Disadvantages: MapPaintStyles.readFromPreferences may be called multiple times. Not a major issue, since this shouldn't be something that is called every minute.

Changed 5 weeks ago by taylor.smock

Attachment: 19196.2.patch added

Fix an issue where the preference listener could be called twice in the same preference change invocation

comment:6 Changed 4 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 16562/josm:

fix #19196 - Don't require a restart when a MapPaint color is changed (patch by taylor.smock)

Changed 4 weeks ago by taylor.smock

Attachment: 19196.3.patch added

Fix unit tests

comment:8 Changed 4 weeks ago by simon04

In 16578/josm:

see #19196 - Fix unit tests (patch by taylor.smock)

comment:9 Changed 2 weeks ago by simon04

Milestone: 20.06

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.