Modify

Opened 10 months ago

Closed 9 months ago

Last modified 7 months 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 10 months ago.
19196.1.patch (1.6 KB) - added by taylor.smock 9 months ago.
Use a listener to get preference change events
19196.2.patch (3.0 KB) - added by taylor.smock 9 months 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 9 months ago.
Fix unit tests

Download all attachments as: .zip

Change History (14)

Changed 10 months ago by taylor.smock

Attachment: 19196.patch added

comment:1 Changed 9 months ago by taylor.smock

Is there any interest for this patch?

comment:2 Changed 9 months 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 9 months 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 9 months 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 9 months ago by taylor.smock

Attachment: 19196.1.patch added

Use a listener to get preference change events

comment:5 Changed 9 months 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 9 months 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 9 months 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 9 months ago by taylor.smock

Attachment: 19196.3.patch added

Fix unit tests

comment:8 Changed 9 months ago by simon04

In 16578/josm:

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

comment:9 Changed 8 months ago by simon04

Milestone: 20.06

comment:10 Changed 7 months ago by taylor.smock

Ticket #16604 has been marked as a duplicate of this ticket.

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.