Opened 6 years ago
Closed 6 years ago
#13309 closed enhancement (fixed)
Caching and notifying preferences
Reported by: | michael2402 | Owned by: | michael2402 |
---|---|---|---|
Priority: | normal | Milestone: | 17.04 |
Component: | Core | Version: | |
Keywords: | gsoc-core | Cc: | Don-vip, bastiK, stoecker |
Description (last modified by )
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)
Change History (43)
Changed 6 years ago by
Attachment: | patch-preferences-listenable-2.patch added |
---|
comment:1 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 6 years ago by
Changed 6 years ago by
Attachment: | patch-fix-13309.patch added |
---|
comment:3 Changed 6 years ago by
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 6 years ago by
Why is the timeout commented in JOSMTestRules?
// statement = new FailOnTimeoutStatement(statement, timeout);
comment:6 Changed 6 years ago by
Thanks for finding that timeout. No, it should not be commented. I did this for debugging.
comment:7 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
ok this is fixed, I didn't apply this part.
comment:8 Changed 6 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Summary: | [Patch] Caching and notifying preferences → Caching and notifying preferences |
this is not, this patch cause 74 unit test failures: jenkins/job/JOSM/1725/testReport/
comment:9 Changed 6 years ago by
Owner: | changed from team to michael2402 |
---|---|
Status: | reopened → new |
comment:12 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
Description: | modified (diff) |
---|
Changed 6 years ago by
Attachment: | patch-latlon-generic.patch added |
---|
Changed 6 years ago by
Attachment: | patch-fix-13309-2.patch added |
---|
Changed 6 years ago by
Attachment: | patch-fix-13309-2.2.patch added |
---|
comment:15 follow-up: 18 Changed 6 years ago by
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:17 Changed 6 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Much better, still 9 tests to fix:
- org.openstreetmap.josm.gui.dialogs.InspectPrimitiveDialogTest.testBuildMapPaintText
- org.openstreetmap.josm.data.PreferencesTest.testColorNameAlpha
- org.openstreetmap.josm.data.PreferencesTest.testColorAlpha
- org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.test_SingletonAccess
- org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.userNameChanged
- org.openstreetmap.josm.gui.JosmUserIdentityManagerTest.apiUrlChanged
- org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSParserTest.testColorNameTicket9191
- org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSParserTest.testColorNameTicket9191Alpha
- org.openstreetmap.josm.tools.bugreport.BugReportExceptionHandlerTest.testHandleException
comment:18 Changed 6 years ago by
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 6 years ago by
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
comment:20 Changed 6 years ago by
Summary: | [Patch] Caching and notifying preferences → Caching and notifying preferences |
---|
comment:23 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:24 Changed 6 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 6 years ago by
Milestone: | 16.08 → 16.09 |
---|
I was working on that one already.
I have two options:
- Simply replace that call
- 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:27 follow-up: 34 Changed 6 years ago by
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 6 years ago by
Milestone: | 16.10 → 16.11 |
---|
comment:31 Changed 6 years ago by
Milestone: | 16.12 → 17.01 |
---|
comment:32 Changed 6 years ago by
Milestone: | 17.01 → 17.02 |
---|
comment:34 Changed 6 years ago by
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 6 years ago by
Milestone: | 17.02 → 17.03 |
---|
comment:37 Changed 6 years ago by
Milestone: | 17.03 → 17.04 |
---|
comment:38 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Patch does not apply anymore, can you please update it?