#18802 closed enhancement (fixed)
MapCSS: refactor/simplify/improve
| Reported by: | simon04 | Owned by: | simon04 |
|---|---|---|---|
| Priority: | normal | Milestone: | 20.03 |
| Component: | Core | Version: | |
| Keywords: | mapcss yourkit performance | Cc: | taylor.smock |
Description
commit 1ab7320176f8a0ab4868297bd36b375855826ed1
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-23 23:11:32 +0100
Extract MapCSSTagCheckerAsserts class
.../data/validation/tests/MapCSSTagChecker.java | 162 +-------------------
.../validation/tests/MapCSSTagCheckerAsserts.java | 163 +++++++++++++++++++++
.../validation/tests/MapCSSTagCheckerTest.java | 6 +-
3 files changed, 168 insertions(+), 163 deletions(-)
commit 7617b34a06075cd6ea41fc4da708c4f87d6be5f6
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-23 23:22:07 +0100
MapCSSTagChecker: simplify throwError/throwWarning/throwOther parsing
.../data/validation/tests/MapCSSTagChecker.java | 32 +++++++---------------
1 file changed, 10 insertions(+), 22 deletions(-)
commit e17190633b0cf657690cc853cfa73da2ad5d1cb6
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-24 20:21:08 +0100
see #18749 - MapCSSTagChecker: no not persist assertions
Check assertions immediately, when needed, but do not keep them.
.../data/validation/tests/MapCSSTagChecker.java | 40 +++++++----
.../validation/tests/MapCSSTagCheckerAsserts.java | 82 ++++++++++++----------
.../validation/tests/MapCSSTagCheckerTest.java | 44 +++++++++---
3 files changed, 101 insertions(+), 65 deletions(-)
commit 30c41b2583c3b0fa82f6f7c33d479879c713ffa4
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-25 17:54:33 +0100
see #18749 - MapCSSTagChecker.TagCheck: use unmodifiable collections
.../data/validation/tests/MapCSSTagChecker.java | 28 ++++++++++++++++++----
src/org/openstreetmap/josm/tools/Utils.java | 17 +++++++++++++
.../org/openstreetmap/josm/tools/UtilsTest.java | 22 +++++++++++++++++
3 files changed, 62 insertions(+), 5 deletions(-)
commit 88775968c3e21a62fdef7fdb46016f707dc8081a
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-25 23:26:35 +0100
Extract org.openstreetmap.josm.gui.mappaint.mapcss.Declaration
.../data/validation/tests/MapCSSTagChecker.java | 2 +-
.../josm/gui/mappaint/mapcss/Declaration.java | 65 ++++++++++++++++++++++
.../josm/gui/mappaint/mapcss/Instruction.java | 2 +-
.../josm/gui/mappaint/mapcss/MapCSSParser.jj | 2 +-
.../josm/gui/mappaint/mapcss/MapCSSRule.java | 60 --------------------
5 files changed, 68 insertions(+), 63 deletions(-)
commit 01200e3a5ed79411f59158729e54de1cc7731883
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-26 00:11:37 +0100
Add MapCSSRule.matches
scripts/TagInfoExtract.java | 2 +-
.../josm/data/validation/tests/MapCSSTagChecker.java | 2 +-
.../openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java | 14 ++++++++++++++
.../josm/gui/mappaint/mapcss/MapCSSParserTest.java | 8 ++++----
4 files changed, 20 insertions(+), 6 deletions(-)
commit 8dd2df58103a06f37b751ebde70509aa8ce7b1e4
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-26 00:35:14 +0100
Add Selector.getConditions
scripts/TagInfoExtract.java | 6 +-----
.../josm/gui/mappaint/mapcss/Selector.java | 17 +++++++++++++----
2 files changed, 14 insertions(+), 9 deletions(-)
commit edc421183618289ad735ff4d05dc2ee84c013c31
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-26 01:08:32 +0100
Add Selector.getBase
.../validation/tests/MapCSSTagCheckerIndex.java | 10 ++----
.../gui/mappaint/mapcss/MapCSSStyleSource.java | 41 ++++++----------------
.../josm/gui/mappaint/mapcss/Selector.java | 18 ++++++++--
3 files changed, 29 insertions(+), 40 deletions(-)
commit 45a243e2df32c2eed6a6febfa6687808759d3d88
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-26 23:03:13 +0100
Selector: merge GeneralSelector/OptimizedGeneralSelector as there is no difference
- GeneralSelector extended OptimizedGeneralSelector
- GeneralSelector.optimizedBaseCheck returned OptimizedGeneralSelector === this
.../data/validation/tests/MapCSSTagChecker.java | 10 +--
.../validation/tests/MapCSSTagCheckerIndex.java | 2 +-
.../gui/mappaint/mapcss/MapCSSStyleSource.java | 7 +-
.../josm/gui/mappaint/mapcss/Selector.java | 82 +++++-----------------
.../gui/mappaint/MapRendererPerformanceTest.java | 2 +-
5 files changed, 27 insertions(+), 76 deletions(-)
commit 92e89d1bfd3d01ab7ae4ba142807210ada14f13d
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-27 00:17:24 +0100
MapCSSRule: support list of selectors
- allows to drop MapCSSTagChecker.GroupedMapCSSRule
- remove code duplication in MapCSSStyleSource/MapCSSTagCheckerIndex w.r.t. MapCSSRuleIndex
scripts/TagInfoExtract.java | 3 +-
.../data/validation/tests/MapCSSTagChecker.java | 93 +-----
.../validation/tests/MapCSSTagCheckerIndex.java | 372 +++++++++++----------
.../josm/gui/mappaint/mapcss/MapCSSParser.jj | 6 +-
.../josm/gui/mappaint/mapcss/MapCSSRule.java | 17 +-
.../gui/mappaint/mapcss/MapCSSStyleSource.java | 172 +++-------
.../mappaint/mapcss/ChildOrParentSelectorTest.java | 2 +-
7 files changed, 276 insertions(+), 389 deletions(-)
commit ebdfa3d0bb10cc0017eab130bf1f1ec9dfbc9c1a
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-27 00:29:09 +0100
MapCSSStyleIndex: rename/move from index
.../data/validation/tests/MapCSSTagChecker.java | 32 +++++++++++++---
.../mappaint/mapcss/MapCSSStyleIndex.java} | 43 ++--------------------
.../gui/mappaint/mapcss/MapCSSStyleSource.java | 3 +-
3 files changed, 31 insertions(+), 47 deletions(-)
commit 710d40fdc99fd01bf6eb8aa8237475475e7b4d0a
Author: Simon Legner <Simon.Legner@gmail.com>
Date: 2020-02-27 23:29:25 +0100
see #18749 - KeyValueRegexpCondition: do not store value as string
src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Attachments (1)
Change History (35)
by , 6 years ago
| Attachment: | 18802.patch added |
|---|
comment:1 by , 6 years ago
| Summary: | MapCSS: refactor/simplify/improve → [Patch] MapCSS: refactor/simplify/improve |
|---|
comment:2 by , 6 years ago
comment:15 by , 6 years ago
comment:16 by , 6 years ago
| Keywords: | yourkit added |
|---|
comment:18 by , 6 years ago
Nice :) I tried that a while ago and failed because of the complexity.
There are a few new complains like unused private method hasSameDeclaration(), I assume this is still work in progress?
See also my new patch for #13165.
comment:22 by , 6 years ago
Test org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[relation-linkselector] broken (most likely) due to r15986 (found using git bisect)
comment:25 by , 6 years ago
I wonder when ruleToCheckMap should be cleared. IIGTR there might be a problem when rules are reloaded.
comment:26 by , 6 years ago
Not a big problem, but it is a memory leak. I think it should be cleared in endTest()
follow-up: 29 comment:28 by , 6 years ago
| Cc: | added |
|---|
Test failure: org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity[MapWithAI - https://josm.openstreetmap.de/wiki/Styles/MapWithAI]
junit.framework.AssertionFailedError: [java.lang.IllegalArgumentException: Unknown settings group: show_all] [] at org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity(MapPaintPreferenceTestIT.java:110)
Is it linked to this ticket?
comment:29 by , 6 years ago
Replying to Don-vip:
Test failure: org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity[MapWithAI - https://josm.openstreetmap.de/wiki/Styles/MapWithAI]
junit.framework.AssertionFailedError: [java.lang.IllegalArgumentException: Unknown settings group: show_all] [] at org.openstreetmap.josm.gui.preferences.map.MapPaintPreferenceTestIT.testStyleValidity(MapPaintPreferenceTestIT.java:110)Is it linked to this ticket?
I have no clue -- I cannot reproduce (the test is passing in Eclipse, via command line (full test run in a clean josm source directory), and via a Ubuntu VM using my working josm directory, with JUnit 5).
Note: I ran the tests with r16048, which had the test fail on Jenkins.
EDIT: I just made some modifications to the MapWithAI paint style. Based off of the feedback from the test (Unknown settings group: show_all), I removed the @supports (min-josm-version: 15289) just for testing purposes. It doesn't make a major difference for the paint style, since its initial revision was 15229.
My guess is that the test doesn't understand @supports(min-josm-version: VERSION) when running under Jenkins CI. Just to double check, I am rerunning the whole test suite in an Ubuntu VM, which should (largely) replicate the Jenkins environment (this time, in a clean working directory).
comment:30 by , 6 years ago
I see it with r16007 for the first time:
https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/4349/
comment:31 by , 6 years ago
| Keywords: | performance added |
|---|---|
| Summary: | [Patch] MapCSS: refactor/simplify/improve → MapCSS: refactor/simplify/improve |
comment:32 by , 6 years ago
I forgot to look at this bug report the day after I made the modification to the MapWithAI paint style, and Jenkins looks like it doesn't keep much more than the past week of commits.
That being said, the Integration Test is now passing. I'm going to go ahead and revert the change I made to the paint style. I hope I remember to look at the tests tomorrow.
comment:33 by , 6 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
In 15979/josm: