Modify

Opened 4 years ago

Closed 5 months ago

Last modified 3 months ago

#18866 closed enhancement (fixed)

[PATCH] Remove Potlatch2 mappaint style from core

Reported by: Stereo Owned by: team
Priority: normal Milestone: 23.11
Component: Core Version:
Keywords: svg icon potlatch2 Cc:

Description

Since we have an icon scaling on hidpi screens now (ticket:9995#comment:110) it would be nice to transform all icons to svg. Preset icons are already all svg (#13357) and GUI icons are making great progress (#15240).

https://www.sjjb.co.uk/mapicons/contactsheet has a lot of icons that we use in the potlatch2 theme - https://www.sjjb.co.uk/mapicons/svg/accommodation/alpinehut.svg could replace source:trunk/resources/images/icons/accommodation_alpinehut.n.16.png resources/images/icons/accommodation_alpinehut.n.16.png etc.

http://osm-icons.org/wiki/Icons has more of the same icons

Attachments (1)

18866.1.patch (5.1 KB ) - added by taylor.smock 5 months ago.
Remove the potlatch2 style from JOSM source

Download all attachments as: .zip

Change History (67)

comment:1 by Stereo, 4 years ago

In https://josm.openstreetmap.de/ticket/15240#comment:99 , stoecker writes:

A zip file with the new images and a small shell script doing the necessary svn commands would probable be the best idea (old icons moved with "svn mv" to nodist directory).

Last edited 3 years ago by Stereo (previous) (diff)

comment:2 by Klumbumbus, 4 years ago

Component: CoreInternal mappaint style

comment:3 by Klumbumbus, 4 years ago

Summary: Transform Potlatch2 theme icons to SVGTransform Potlatch2 mappaint style icons to SVG

Before changing the icons in JOSM maybe first ask the P2 guys if they are interested in switching to svg. Then the change can be made there first.

comment:4 by Stereo, 4 years ago

Yeah, I notice that the P2 style hasn't been pulled since 2016, and that the patches could, in some cases, be backported.

I'll check with Richard.

comment:5 by Don-vip, 4 years ago

Keywords: svg icon potlatch2 added

comment:6 by Stereo, 4 years ago

Richard has no plans to convert the Potlatch2 style to SVG since Flash doesn't support it. I didn't get the feeling that he was planning any changes on it, especially considering Flash's imminent demise.

Are there any reasons other than historical for which the Potlatch2 style is built-in and not a custom style like the rest of the other styles?

in reply to:  6 comment:7 by Klumbumbus, 4 years ago

Replying to Stereo:

Are there any reasons other than historical for which the Potlatch2 style is built-in and not a custom style like the rest of the other styles?

Probably because it needs to be patched to work in JOSM.

comment:8 by Don-vip, 4 years ago

Also, the P2 style is a general style, not a feature-specific one.

comment:9 by Stereo, 4 years ago

Component: Internal mappaint styleCore

Since Richard doesn't intend to work on it anymore, (one day) I will take the patched version, integrate any changes done upstream since, change the icons to vectors and host it on the wiki here, like https://josm.openstreetmap.de/wiki/Styles/iD

comment:10 by anonymous, 4 years ago

Status: assignednew

comment:11 by stoecker, 4 years ago

I'd say the main reason is historical. There is no need to have it internal.

comment:12 by Klumbumbus, 4 years ago

In 17016/josm:

see #18866 - Update to latest and probably last version of Potlatch2 mappaint style

comment:14 by Klumbumbus, 4 years ago

In 17022/josm:

comment:15 by GerdP, 4 years ago

I have a question reg. the icon names: Is the .svg extension wanted? My understanding was that ImageProvider should control what kind of format it uses if there are alternatives.

in reply to:  15 comment:16 by Klumbumbus, 4 years ago

Replying to GerdP:

I have a question reg. the icon names: Is the .svg extension wanted? My understanding was that ImageProvider should control what kind of format it uses if there are alternatives.

--> #19788

comment:17 by Stereo, 3 years ago

https://josm.openstreetmap.de/wiki/Styles/Potlatch2

The built-in P2 style can now be deleted. There should probably be a note in the next motd to tell people to activate the vectorised style.

comment:19 by Klumbumbus, 3 years ago

JOSM can't render the bakery icon:

2020-10-29 20:43:31.387 WARNING: Could not load SVG svgSalamander:/icons/shopping_bakery.n.svg
java.lang.NumberFormatException: For input string: "l"
	at sun.misc.FloatingDecimal.readJavaFormatString(Unknown Source)
	at sun.misc.FloatingDecimal.parseFloat(Unknown Source)
	at java.lang.Float.parseFloat(Unknown Source)
	at com.kitfox.svg.SVGElement.nextFloat(SVGElement.java:687)
	at com.kitfox.svg.SVGElement.parsePathList(SVGElement.java:758)
	at com.kitfox.svg.SVGElement.buildPath(SVGElement.java:811)
	at com.kitfox.svg.Path.build(Path.java:87)
	at com.kitfox.svg.Path.updateTime(Path.java:151)
	at com.kitfox.svg.Group.updateTime(Group.java:313)
	at com.kitfox.svg.Group.updateTime(Group.java:313)
	at com.kitfox.svg.SVGRoot.updateTime(SVGRoot.java:409)
	at com.kitfox.svg.SVGDiagram.updateTime(SVGDiagram.java:243)
	at com.kitfox.svg.SVGUniverse.loadSVG(SVGUniverse.java:616)
	at com.kitfox.svg.SVGUniverse.loadSVG(SVGUniverse.java:499)
	at com.kitfox.svg.SVGUniverse.loadSVG(SVGUniverse.java:484)
	at org.openstreetmap.josm.tools.ImageProvider.getIfAvailableZip(ImageProvider.java:1124)
	at org.openstreetmap.josm.tools.ImageProvider.getIfAvailableImpl(ImageProvider.java:937)
	at org.openstreetmap.josm.tools.ImageProvider.getResource(ImageProvider.java:712)
	at org.openstreetmap.josm.tools.ImageProvider.getResourceAsync(ImageProvider.java:747)
	at org.openstreetmap.josm.gui.mappaint.styleelement.MapImage.load(MapImage.java:175)
	at org.openstreetmap.josm.gui.mappaint.styleelement.MapImage.loadImage(MapImage.java:184)
	at org.openstreetmap.josm.gui.mappaint.styleelement.MapImage.getImage(MapImage.java:158)
	at org.openstreetmap.josm.gui.mappaint.styleelement.MapImage.getWidth(MapImage.java:230)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.drawNodeIcon(StyledMapRenderer.java:791)
	at org.openstreetmap.josm.gui.mappaint.styleelement.NodeElement.paintPrimitive(NodeElement.java:260)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$StyleRecord.paintPrimitive(StyledMapRenderer.java:226)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.paintRecord(StyledMapRenderer.java:1713)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.paintWithLock(StyledMapRenderer.java:1695)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.render(StyledMapRenderer.java:1644)
	at org.openstreetmap.josm.gui.layer.OsmDataLayer.paint(OsmDataLayer.java:543)
	at org.openstreetmap.josm.gui.layer.AbstractMapViewPaintable$CompatibilityModeLayerPainter.paint(AbstractMapViewPaintable.java:27)
	at org.openstreetmap.josm.gui.MapView.paintLayer(MapView.java:466)
	at org.openstreetmap.josm.gui.MapView.drawMapContent(MapView.java:581)
	at org.openstreetmap.josm.gui.MapView.paint(MapView.java:488)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JSplitPane.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JLayeredPane.paint(Unknown Source)
	at javax.swing.JComponent.paintChildren(Unknown Source)
	at javax.swing.JComponent.paintToOffscreen(Unknown Source)
	at javax.swing.RepaintManager$PaintManager.paintDoubleBuffered(Unknown Source)
	at javax.swing.RepaintManager$PaintManager.paint(Unknown Source)
	at javax.swing.RepaintManager.paint(Unknown Source)
	at javax.swing.JComponent.paint(Unknown Source)
	at java.awt.GraphicsCallback$PaintCallback.run(Unknown Source)
	at sun.awt.SunGraphicsCallback.runOneComponent(Unknown Source)
	at sun.awt.SunGraphicsCallback.runComponents(Unknown Source)
	at java.awt.Container.paint(Unknown Source)
	at java.awt.Window.paint(Unknown Source)
	at javax.swing.RepaintManager$4.run(Unknown Source)
	at javax.swing.RepaintManager$4.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
	at javax.swing.RepaintManager.paintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.paintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.prePaintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.access$1200(Unknown Source)
	at javax.swing.RepaintManager$ProcessingRunnable.run(Unknown Source)
	at java.awt.event.InvocationEvent.dispatch(Unknown Source)
	at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
	at java.awt.EventQueue.access$500(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue.dispatchEvent(Unknown Source)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.run(Unknown Source)

2020-10-29 20:43:31.392 SEVERE: Failed to locate image 'icons/shopping_bakery.n.svg'
2020-10-29 20:44:07.717 SEVERE: createImageFromSvg: svgSalamander:/icons/shopping_bakery.n.svg java.awt.Dimension[width=-1,height=-1] sourceWidth=0 sourceHeight=0

comment:20 by Stereo, 3 years ago

Wow, well spotted. Even the original didn't work. I fixed it by re-saving the icon, then re-minifying it.

Do you have a test script to build .osm files, or did you test them all by hand?

Thanks!

in reply to:  20 comment:21 by Klumbumbus, 3 years ago

Replying to Stereo:

Do you have a test script to build .osm files, or did you test them all by hand?

No, if there is a problem with the style then it has a exclamation mark on top of its icon. Then you can right click -> info and then is a hint in the Errors or Warnings tab.

wiki:/Help/Dialog/MapPaint#Info

Last edited 3 years ago by Klumbumbus (previous) (diff)

comment:22 by Klumbumbus, 3 years ago

Owner: changed from Stereo to team

Removing the old core Potlatch2 mappaint style requires some code to remove it from the users list of active styles.

comment:23 by Stereo, 3 years ago

Milestone: 20.10

If a message can be added to the MOTD, the built-in P2 style can be removed in 20.10.

comment:24 by Klumbumbus, 3 years ago

Another thing: at least one external styles uses the core potlatch pngs: wiki:/Styles/PowerMapping

comment:26 by Klumbumbus, 3 years ago

Good, they a pretty small. One could add the new svgs there too.

comment:27 by Stereo, 3 years ago

Done!

I don't have the permissions to delete, overwrite or update the old Icons_JP-Tsunami.zip​ attachment on https://josm.openstreetmap.de/wiki/Styles/JP-Tsunami - do you?

"You don't have permission to replace the attachment Icons_JP-Tsunami.zip. You can only replace your own attachments. Replacing other's attachments requires ATTACHMENT_DELETE permission."

comment:28 by Stereo, 3 years ago

So I guess we can now mv resources/images/icons resources/styles/standard/potlatch2.mapcss resources/images/dialogs/mappaint/pl2_small.svg nodist/?

in reply to:  27 comment:29 by Klumbumbus, 3 years ago

Replying to Stereo:

I don't have the permissions to delete, overwrite or update the old Icons_JP-Tsunami.zip​ attachment on https://josm.openstreetmap.de/wiki/Styles/JP-Tsunami - do you?

There are a lot less icons in Icons_JP-Tsunami.2.zip I guess you removed the unneeded ones? I think even if they are not used by the style atm it is better to keep them in the zip, else they are lost.

comment:30 by Stereo, 3 years ago

Yeah, I only kept the ones that were actually being used. I've also optimised the PNGs, saving a few KB.

I suppose the old zip can be renamed instead. I don't have the rights to do that either.

comment:31 by Stereo, 3 years ago

Cc: pierzen added

Adding Pierzen, the author of the Tsunami style, in CC.

comment:32 by Klumbumbus, 3 years ago

I reuploaded the original file with updated description and with (NOZIP)

https://josm.openstreetmap.de/attachment/wiki/Styles/JP-Tsunami/Icons_JP-Tsunami.zip

Last edited 3 years ago by Klumbumbus (previous) (diff)

comment:33 by Stereo, 3 years ago

Cc: pierzen removed

Perfect, removing PierZen then.

comment:34 by Klumbumbus, 3 years ago

Milestone: 20.1020.11

comment:35 by Don-vip, 3 years ago

Milestone: 20.1120.12

Milestone renamed

comment:36 by Stereo, 3 years ago

What remains to be done on this ticket is mv resources/images/icons resources/styles/standard/potlatch2.mapcss resources/images/dialogs/mappaint/pl2_small.svg nodist/

comment:37 by Klumbumbus, 3 years ago

comment:38 by simon04, 3 years ago

Milestone: 20.1221.01

comment:39 by Don-vip, 3 years ago

Milestone: 21.0121.02

comment:40 by stoecker, 3 years ago

Milestone: 21.0221.03

Milestone renamed

comment:41 by Don-vip, 3 years ago

Milestone: 21.03

comment:42 by Stereo, 3 years ago

Milestone: 21.04

comment:43 by simon04, 3 years ago

Milestone: 21.04Longterm

in reply to:  22 ; comment:44 by taylor.smock, 5 months ago

Replying to Klumbumbus:

Removing the old core Potlatch2 mappaint style requires some code to remove it from the users list of active styles.

Did we have any other objections to removing Potlatch2 from the built-in paintstyles? If not, I'll see if I can code something up for that in the next few weeks.

comment:45 by stoecker, 5 months ago

There is a config sanitizer for such cases:

Preferences.removeAndUpdateObsolete()

comment:46 by taylor.smock, 5 months ago

It looks like we are doing this in source:trunk/src/org/openstreetmap/josm/data/preferences/sources/MapPaintPrefHelper.java (we still have a section removing elemstyles.xml).

in reply to:  44 comment:47 by Klumbumbus, 5 months ago

Replying to taylor.smock:

Did we have any other objections to removing Potlatch2 from the built-in paintstyles?

No.

Replying to stoecker:

There is a config sanitizer for such cases:

Preferences.removeAndUpdateObsolete()

Then I guess it is pretty easy to implement it.

Additional todos:

  • move icon folder, mapcss file and logo file to nodist or delete it, see comment:36
  • the following folder is already in nodist, I'm not sure if this should be deleted. It is not useful anymore as potlatch2 is dead anyway, I doubt there will be updates of the style in the future.

by taylor.smock, 5 months ago

Attachment: 18866.1.patch added

Remove the potlatch2 style from JOSM source

comment:48 by taylor.smock, 5 months ago

Milestone: Longterm23.10
Summary: Transform Potlatch2 mappaint style icons to SVG[PATCH] Remove Potlatch2 mappaint style from core

I've got the trunk/resources/images and trunk/nodist/styles/potlatch2 directories deleted locally, they just didn't show up in the patch file.

The patch tries to ensure that if someone has Potlatch2 enabled, they still have a Potlatch2 style after the update.

I'll plan on applying the patch in a week or two to give people a chance to look it over.

comment:49 by stoecker, 5 months ago

Please add an expire comment (I'd say at least 6 months), so the code gets removed later.

comment:50 by stoecker, 5 months ago

Example: /* Remove this code block and the related function after 2024-06-01 */

Last edited 5 months ago by stoecker (previous) (diff)

comment:51 by taylor.smock, 5 months ago

Fair point. I should probably go through other stuff we've deprecated and remove it. As an example, I've been meaning to go through all @Deprecated methods as well and add a since xxx to the @deprecated javadoc so we can have some kind of schedule to remove those.

comment:52 by stoecker, 5 months ago

We'll that's easy. Remove all Deprecated in January together with the Java 11 upgrade. Until then all deprecate usages should be fixed thought...

Last edited 5 months ago by stoecker (previous) (diff)

comment:53 by taylor.smock, 5 months ago

Resolution: fixed
Status: newclosed

In 18886/josm:

Fix #18866: Remove Potlatch2 from the built-in styles. It has been replaced with wiki:Styles/Potlatch2.

Users who have the built-in Potlatch2 style enabled when they upgrade to this
revision or later will automatically have wiki:Styles/Potlatch2 added to their
style list and enabled.

comment:54 by Klumbumbus, 5 months ago

Fixed the one icon path in the 5 power line mapping presets.
Now there is one style left which used the icons. I created: https://github.com/hotosm/HDM-JOSM-style/issues/72

comment:55 by taylor.smock, 5 months ago

In 18888/josm:

See #18866: Remove Potlatch2 from the built-in styles

This fixes the MapRendererPerformanceTest that failed due to depending upon the
number of built-in paintstyles.

in reply to:  54 comment:56 by taylor.smock, 5 months ago

Replying to Klumbumbus:

Fixed the one icon path in the 5 power line mapping presets.
Now there is one style left which used the icons. I created: https://github.com/hotosm/HDM-JOSM-style/issues/72

Thanks for doing that -- I was going to take a look after I finished fixing the borked test.

comment:57 by gaben, 5 months ago

Hmm, for me on the dev machine it removed all the styles, even the built-in JOSM MapCSS for some reason.

I think I'm out of luck as the preferences cleanup is a one-time operation. I had a test file added to the run parameters, that's what I can say at the moment.

Last edited 5 months ago by gaben (previous) (diff)

comment:58 by taylor.smock, 5 months ago

I think I'm out of luck as the preferences cleanup is a one-time operation

I encounter this frequently after running JOSM tests. I've been slowly fixing the root cause. Did you run any JOSM tests? Anything that has been fully migrated to JUnit5 annotations should be safe, but anything using JOSMTestRules and JOSMFixture are not.

EDIT: To clarify, in my testing with many additional paintstyles, the paint styles were not reset. It sounds like your preferences were fully reset ("preferences cleanup is a one-time operation"). That frequently occurs to me when I run unit tests. If that isn't correct, feel free to yell at me. And then I'll revert r18888.

Last edited 5 months ago by taylor.smock (previous) (diff)

comment:59 by gaben, 5 months ago

Yes, I was running some tests, but as far as I know all of them were already migrated to JUnit5.

No serious issue here. It's a dev machine and the preferences are intact, except the mappaint style settings.

Upon restart, I realized neither the wireframe view option was there after the cleanup routine did its job. The wireframe option is not even a downloadable thing but a built-in feature. Really weird. I'm not reopening this ticket just yet, but it needs some more testing.

When is the next planned stable release?

comment:60 by taylor.smock, 5 months ago

When is the next planned stable release?

Whenever we get a new code signing certificate set up. See #23107 for details.

Yes, I was running some tests, but as far as I know all of them were already migrated to JUnit5.

There is a reason why I specified that the tests that have been fully migrated to our JUnit 5 annotations were safe. A test that uses JOSMTestRules (which works on both JUnit 4 and 5) or JOSMFixture will bork your preferences. I'm going through the remaining tests that use either and updating it.

Tests that can cause a reset to preferences:

  • CertificateAmendmentTestIT (JOSMTestRules)
  • DomainValidatorTestIT (JOSMTestRules)
  • GpxLayerTest (JOSMTestRules)
  • HelpBrowserTest (JOSMTestRules)
  • ImageryPreferenceTestIT (JOSMTestRules)
  • LifecycleTest (JOSMTestRules)
  • MainApplicationTest (JOSMTestRules)
  • MainLayerManagerTest (JOSMFixture)
  • MapCSSParserTestIT (JOSMTestRules)
  • MapPaintPreferenceTestIT (JOSMTestRules)
  • MemoryManagerTest (JOSMTestRules)
  • MinimapDialogTest (JOSMTestRules)
  • NetworkManagerTest (JOSMTestRules)
  • OsmServerBackreferenceReaderTest (JOSMFixture)
  • PlatformHookWindowsTest (JOSMTestRules)
  • PluginDownloadTaskTest (JOSMTestRules)
  • PluginHandlerTestIT (JOSMTestRules)
  • RemoteControlTest (JOSMTestRules)
  • SignpostAdaptersTest (JOSMTestRules)
  • TaggingPresetPreferenceTestIT (JOSMTestRules)
  • WebMarkerTest (JOSMTestRules)
  • And a few other tests I haven't modified today, but still need to (mostly JOSMTestRules).

comment:61 by taylor.smock, 5 months ago

No serious issue here.

To clarify, if new code is clearing out preferences, something is wrong and I need to fix it ASAP.

It's a dev machine and the preferences are intact, except the mappaint style settings.

The "except mappaint style settings" would tend to make me blame this ticket, except I often have my style preferences wiped after running tests. I'm working on fixing that so I know whether or not it was tests or new code that caused the problem in the future.

Upon restart, I realized neither the wireframe view option was there after the cleanup routine did its job. The wireframe option is not even a downloadable thing but a built-in feature.

Are you talking about View -> Wireframe View? Or it doesn't work when you enable it?
That button is provided by code, and I have no clue why it wouldn't have appeared.

comment:62 by gaben, 5 months ago

Today it happened once again. I was just running some 5-8 years old versions this time, no tests at all.

I've put some log statements in the cleanup method to see what's going on if it happens again. It may not be related at all, I still have no clue.

comment:63 by taylor.smock, 4 months ago

I have no clue how that is happening. If I cannot figure it out this week, I'll revert this patch.

Literally the only paintstyles that should be removed/modified are those with a url == "resource://styles/standard/potlatch2.mapcss".

comment:64 by taylor.smock, 4 months ago

Milestone: 23.1023.11

Ticket retargeted after milestone deleted

comment:65 by taylor.smock, 4 months ago

In 18898/josm:

See #18866: Remove Potlatch2 from the built-in styles

Remove an icon that was missed for the Potlatch2 style.

comment:66 by gaben, 3 months ago

Is there anything to do in this ticket? I think what I found in comment:57 has a separate ticket (and hotfix r18907) now, see #23341.

Edit: Okay, it's already closed, never mind :) I thought I reopened, but seems I forgot.

Last edited 3 months ago by gaben (previous) (diff)

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.