#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 , 5 years ago
Attachment: | 18802.patch added |
---|
comment:1 by , 5 years ago
Summary: | MapCSS: refactor/simplify/improve → [Patch] MapCSS: refactor/simplify/improve |
---|
comment:2 by , 5 years ago
comment:15 by , 5 years ago
comment:16 by , 5 years ago
Keywords: | yourkit added |
---|
comment:18 by , 5 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 , 5 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 , 5 years ago
I wonder when ruleToCheckMap
should be cleared. IIGTR there might be a problem when rules are reloaded.
comment:26 by , 5 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 , 5 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 , 5 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 , 5 years ago
I see it with r16007 for the first time:
https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/4349/
comment:31 by , 5 years ago
Keywords: | performance added |
---|---|
Summary: | [Patch] MapCSS: refactor/simplify/improve → MapCSS: refactor/simplify/improve |
comment:32 by , 5 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 , 5 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
In 15979/josm: