Modify

Opened 8 months ago

Last modified 8 days ago

#13309 reopened enhancement

Caching and notifying preferences

Reported by: michael2402 Owned by: michael2402
Priority: normal Milestone: 17.03
Component: Core Version:
Keywords: gsoc-core Cc: Don-vip, bastiK, stoecker

Description (last modified by michael2402)

Currently, there are many MapFrame.repaint() calls whenever properties change.

Part of this is because colors and other values are cached by each component seperately.

I added a new infrastructure that allows you to get a property and easily cache that result.

To get a property use:

new IntegerProperty("my.property", "123").get()

To cache the result, use:

prop = new IntegerProperty("my.property", "123").cached(); // registers 123 as default value
prop.get(); // lazy get and convert to int
prop.get(); // get cached integer object

I added a new preference listener that listens to changes of one specific key to make this efficient.

Attachments (5)

patch-preferences-listenable-2.patch (86.8 KB) - added by michael2402 8 months ago.
patch-fix-13309.patch (86.8 KB) - added by michael2402 8 months ago.
patch-latlon-generic.patch (45.4 KB) - added by michael2402 7 months ago.
patch-fix-13309-2.patch (14.2 KB) - added by michael2402 7 months ago.
patch-fix-13309-2.2.patch (14.2 KB) - added by michael2402 7 months ago.

Download all attachments as: .zip

Change History (41)

Changed 8 months ago by michael2402

comment:1 Changed 8 months ago by simon04

Description: modified (diff)

comment:2 Changed 8 months ago by Don-vip

Patch does not apply anymore, can you please update it?

Changed 8 months ago by michael2402

Attachment: patch-fix-13309.patch added

comment:3 Changed 7 months ago by Don-vip

    public static class ValueChangeEvent<T> {
        private final PreferenceChangeEvent base;

        private final AbstractProperty<T> source;

        ValueChangeEvent(PreferenceChangeEvent base, AbstractProperty<T> source) {
            this.base = base;
            this.source = source;
        }

base is not used?

comment:4 Changed 7 months ago by Don-vip

Why is the timeout commented in JOSMTestRules?

           //  statement = new FailOnTimeoutStatement(statement, timeout);

comment:5 Changed 7 months ago by Don-vip

In 10824/josm:

see #13309 - Caching and notifying preferences (patch by michael2402) - gsoc-core

comment:6 Changed 7 months ago by michael2402

Thanks for finding that timeout. No, it should not be commented. I did this for debugging.

comment:7 Changed 7 months ago by Don-vip

Resolution: fixed
Status: newclosed

ok this is fixed, I didn't apply this part.

comment:8 Changed 7 months ago by Don-vip

Resolution: fixed
Status: closedreopened
Summary: [Patch] Caching and notifying preferencesCaching and notifying preferences

this is not, this patch cause 74 unit test failures: jenkins/job/JOSM/1725/testReport/

comment:9 Changed 7 months ago by Don-vip

Owner: changed from team to michael2402
Status: reopenednew

comment:10 Changed 7 months ago by Don-vip

In 10839/josm:

see #13309 - remove deprecated color stuff not used outside of JOSM

comment:11 Changed 7 months ago by Don-vip

In 10853/josm:

see #13309 - fix most of deprecation warnings

comment:12 Changed 7 months ago by Don-vip

it seems the exception causing all these test failures is

java.lang.NullPointerException
	at org.openstreetmap.josm.data.preferences.AbstractToStringProperty.getAsString(AbstractToStringProperty.java:131)
	at org.openstreetmap.josm.data.preferences.AbstractToStringProperty.get(AbstractToStringProperty.java:92)
	at org.openstreetmap.josm.data.preferences.BooleanProperty.get(BooleanProperty.java:21)
	at org.openstreetmap.josm.tools.date.DateUtils.getDateTimeFormat(DateUtils.java:311)
	at org.openstreetmap.josm.gui.dialogs.NotesDialog$NoteRenderer.<init>(NotesDialog.java:209)
	at org.openstreetmap.josm.gui.dialogs.NotesDialog$NoteRenderer.<init>(NotesDialog.java:206)
	at org.openstreetmap.josm.gui.dialogs.NotesDialog.buildDialog(NotesDialog.java:85)
	at org.openstreetmap.josm.gui.dialogs.NotesDialog.<init>(NotesDialog.java:78)
	at org.openstreetmap.josm.gui.MapFrame.<init>(MapFrame.java:264)
	at org.openstreetmap.josm.gui.MainPanel.createNewMapFrame(MainPanel.java:88)
	at org.openstreetmap.josm.gui.MainPanel.updateContent(MainPanel.java:63)
	at org.openstreetmap.josm.gui.MainPanel$1.beforeFirstLayerAdded(MainPanel.java:157)
	at org.openstreetmap.josm.gui.layer.MainLayerManager.realAddLayer(MainLayerManager.java:264)
	at org.openstreetmap.josm.gui.layer.LayerManager.lambda$addLayer$0(LayerManager.java:180)
	at org.openstreetmap.josm.gui.util.GuiHelper.runInEDTAndWaitWithException(GuiHelper.java:138)
	at org.openstreetmap.josm.gui.layer.LayerManager.addLayer(LayerManager.java:180)
	at org.openstreetmap.josm.JOSMFixture.setupGUI(JOSMFixture.java:154)
	at org.openstreetmap.josm.JOSMFixture.access$000(JOSMFixture.java:28)
	at org.openstreetmap.josm.JOSMFixture$1.run(JOSMFixture.java:128)

comment:13 Changed 7 months ago by michael2402

The problem is that some class loads DateUtils before Main.pref is instantiated.

I suggest fixing this and adding a dependency on preferences for that test. I'll find the test that causes this and post a patch for it.

An alternative to make our life easier would be to make Main.pref a final field instantiated with an empty instance. We can then simply load it during program startup. This would also make early application startup much easier, since we currently have many classes that may only be loaded after Main.pref is set. We should make those classes listen to preferences updates though, but this is why I created this patch.

comment:14 Changed 7 months ago by michael2402

Description: modified (diff)

Changed 7 months ago by michael2402

Attachment: patch-latlon-generic.patch added

Changed 7 months ago by michael2402

Attachment: patch-fix-13309-2.patch added

Changed 7 months ago by michael2402

Attachment: patch-fix-13309-2.2.patch added

comment:15 Changed 7 months ago by michael2402

Summary: Caching and notifying preferences[Patch] Caching and notifying preferences

Please ignore patch-latlon-generic.patch​ and patch-fix-13309-2.2.patch​. This happens if you have 4 track windows open because it has a load time of 2 Minutes (if it loads)

I attached patch-fix-13309-2.patch​ to address the issue. It makes the pref field final. That field was only changed in tests and should never be changed while JOSM is running, so it should not be a problem.

comment:16 Changed 7 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 10876/josm:

fix #13309 - fix unit tests (patch by michael2402) - gsoc-core

comment:17 Changed 7 months ago by Don-vip

Resolution: fixed
Status: closedreopened

Much better, still 9 tests to fix:

  1. org.openstreetmap.josm.gui.dialogs.InspectPrimitiveDialogTest.testBuildMapPaintText
  2. org.openstreetmap.josm.data.PreferencesTest.testColorNameAlpha
  3. org.openstreetmap.josm.data.PreferencesTest.testColorAlpha
  4. org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.test_SingletonAccess
  5. org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.userNameChanged
  6. org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.apiUrlChanged
  7. org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSParserTest.testColorNameTicket9191
  8. org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSParserTest.testColorNameTicket9191Alpha
  9. org.openstreetmap.josm.tools.bugreport.BugReportExceptionHandlerTest.testHandleException

comment:18 in reply to:  15 Changed 7 months ago by Don-vip

Replying to michael2402:

because it has a load time of 2 Minutes (if it loads)

website was targeted by what seems to be a DDoS, I found a solution, it is now much faster.

comment:19 Changed 7 months ago by Don-vip

Deprecation warnings must also be fixed:

PropertiesCellRenderer.java:37, Java Compiler (javac), Priority: Normale
getColor(String,Color) in Preferences has been deprecated

PropertiesCellRenderer.java:38, Java Compiler (javac), Priority: Normale
getColor(String,Color) in Preferences has been deprecated

PropertiesCellRenderer.java:41, Java Compiler (javac), Priority: Normale
getColor(String,Color) in Preferences has been deprecated

PropertiesCellRenderer.java:42, Java Compiler (javac), Priority: Normale
getColor(String,Color) in Preferences has been deprecated

MapCSSParser.java:2025, Java Compiler (javac), Priority: Normale
getColor(String,Color) in Preferences has been deprecated
MapView.java:869, Java Compiler (javac), Priority: Normale
isChanged() in Layer has been deprecated

AbstractTileSourceLayer.java:1853, Java Compiler (javac), Priority: Normale
isChanged() in Layer has been deprecated

GpxLayer.java:259, Java Compiler (javac), Priority: Normale
isChanged() in Layer has been deprecated

OsmDataLayer.java:896, Java Compiler (javac), Priority: Normale
isChanged() in Layer has been deprecated

ValidatorLayer.java:126, Java Compiler (javac), Priority: Normale
isChanged() in Layer has been deprecated
Last edited 7 months ago by Don-vip (previous) (diff)

comment:20 Changed 7 months ago by Don-vip

Summary: [Patch] Caching and notifying preferencesCaching and notifying preferences

comment:21 Changed 7 months ago by michael2402

In 10920/josm:

See #13309: Do not add 'color.' prefix twice in getColor()

comment:22 Changed 7 months ago by michael2402

In 10922/josm:

See #13309: Make PreferencesTest compile again.

comment:23 Changed 7 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

comment:24 Changed 7 months ago by Don-vip

Resolution: fixed
Status: closedreopened

one remaining deprecation warning to fix in MapCSSParser.jj:

        { return Main.pref.getColor("mappaint." + (sheet == null ? "MapCSS" : sheet.title) + "." + pref, ColorHelper.html2color(t.image)); }

comment:25 Changed 7 months ago by michael2402

Milestone: 16.0816.09

I was working on that one already.

I have two options:

  1. Simply replace that call
  2. Use a color property as return value - that way, we can reload preferences easier

I'd like to apply Option 2 after the release

comment:26 Changed 6 months ago by simon04

Milestone: 16.0916.10

Milestone renamed

comment:27 Changed 5 months ago by simon04

Since recently, opening the preferences outputs the following warnings:

Last errors/warnings:
- W: Unable to get color from '' for color preference 'extrude.main.line'
- W: Unable to get color from '' for color preference 'improve.way.accuracy.helper.line'
- W: Unable to get color from '' for color preference 'make.parallel.helper.line'

comment:28 Changed 5 months ago by simon04

Milestone: 16.1016.11

comment:29 Changed 4 months ago by michael2402

In 11270/josm:

See #13309: Make download dialog use preferences interface.

comment:30 Changed 4 months ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

comment:31 Changed 3 months ago by Don-vip

Milestone: 16.1217.01

comment:32 Changed 2 months ago by Don-vip

Milestone: 17.0117.02

comment:33 Changed 5 weeks ago by Don-vip

#14343#comment:10 must be addressed as well.

comment:34 in reply to:  27 Changed 5 weeks ago by Klumbumbus

Replying to simon04:

Since recently, opening the preferences outputs the following warnings:

Last errors/warnings:
- W: Unable to get color from '' for color preference 'extrude.main.line'
- W: Unable to get color from '' for color preference 'improve.way.accuracy.helper.line'
- W: Unable to get color from '' for color preference 'make.parallel.helper.line'

I think this was fixed in r11542

comment:35 Changed 4 weeks ago by Don-vip

Milestone: 17.0217.03

comment:36 Changed 8 days ago by michael2402

In 11758/josm:

See #13309: Use cached preferences for map drawing; align box text to full pixels if not rotated.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain michael2402.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from michael2402 to the specified user. Next status will be 'new'.
Next status will be 'needinfo'.The owner will change to michael2402
as duplicate The resolution will be set to duplicate. Next status will be 'closed'.The specified ticket will be cross-referenced with this ticket

Add Comment


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

 
Note: See TracTickets for help on using tickets.