Modify

Opened 10 years ago

Closed 9 years ago

#8686 closed enhancement (fixed)

[PATCH] Allow ZIP files whose paths are relative to the ZIP root in JOSM's map styling

Reported by: skorasaurus Owned by: team
Priority: normal Milestone:
Component: Core mappaint Version: latest
Keywords: zip mapcss Cc: ybon

Description (last modified by Don-vip)

HOT OSM is making a map styling for josm and would like to distribute it through a zip file since many of our users will be in areas with limited and sporadic internet connections.

We're also currently using github to manage it and Github automatically generates zip files based on the tag, (i.e. : https://github.com/hotosm/HDM-JOSM-style/archive/0.0.1.zip

Unfortunately, these zip files aren't usable in josm for the map coloring because the icons path is relative to the zip root, not to the file itself. Using the default zip export from github could make the workflow much easier

If you could, we'd greatly appreciate it.

Attachments (4)

patch_8686_1.diff (1.3 KB) - added by dommage 9 years ago.
Patch to allow more lenient searching of style zip files for icons.
patch_8686_2.diff (1.4 KB) - added by dommage 9 years ago.
Updated patch that tries exact path match first.
patch_8686_3.diff (1.4 KB) - added by dommage 9 years ago.
Prevent matching files with the same postfix.
patch_8686_4.diff (1.8 KB) - added by dommage 9 years ago.
If entry not found, search relative to the mapcss file location.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 years ago by skorasaurus

Component: Coreunspecified

comment:2 Changed 10 years ago by Don-vip

Component: unspecifiedCore mappaint
Description: modified (diff)

comment:3 Changed 10 years ago by Don-vip

Type: defectenhancement

comment:4 Changed 10 years ago by stoecker

Owner: changed from team to skorasaurus
Status: newneedinfo

Why don't you simply change your mapcss file, so path is correct?

comment:5 Changed 10 years ago by yohanboniface

@stoecker: Github create a zip of the *folder* of the project, so this means that the root of the git project is *in* a folder in the zip.
For example:

MyZip /
 |____MyProjName /
         |_________my-mapcss-file-usually-at-git-repo-root.txt
         |_________icons/
                     |_________my-icon1.png
                     |_________my-icon2.png

This is why resolving the icons path relative to the mapcss file instead of to the ZIP root can solve the problem.

comment:6 Changed 10 years ago by stoecker

Yes, I understand this. Though the issue remains. Why don't you simply fix the mapcss, so that the path is correct?

comment:7 Changed 10 years ago by yohanboniface

You mean for example using MyProjName/icons/my-icon1.png instead of icons/my-icon1.png as icon-image value ?

If this is what you mean, the answer is: doing that way, the path isn't correct any more when pointing from JOSM directly to the mapcss file. Which is for example the case when you work on the style itself.

And by the way, I think we can say that having the file paths relative to a CSS like stylesheet is the standard way to handle this.
I mean, in pure CSS, if you have the path icons/my-icon.png, this means relatively to the CSS stylesheet path. Otherwise, the path must be absolute (/icons/my-icon.png or /mypath/icons/my-icon.png). So I think following this in JOSM makes sense, no?

comment:8 Changed 10 years ago by bastiK

I agree that it makes more sense to consider the path relative to the mapcss file. As the style is usually in the root of the zip file, we shouldn't break to much with this change.

Last edited 10 years ago by bastiK (previous) (diff)

comment:9 Changed 10 years ago by skyper

@yohanboniface:

Please, either add an external link to Styles or add the style to the wiki but not both.

Thanks

comment:10 Changed 10 years ago by yohanboniface

Well, I've asked don-vip on IRC to delete the wiki page, but he seems idle.
Can you do it for us? Thanks.
BTW, are you on #josm?

comment:11 in reply to:  10 Changed 10 years ago by skyper

Replying to yohanboniface:

Well, I've asked don-vip on IRC to delete the wiki page, but he seems idle.
Can you do it for us? Thanks.

Yes, I can do it.

BTW, are you on #josm?

sometimes.

EDT: Why did you use ubuntu one and not github as link.

EDT2: Sorry, reason is probably this ticket. Am I right.

Last edited 10 years ago by skyper (previous) (diff)

comment:12 Changed 10 years ago by yohanboniface

EDT: Why did you use ubuntu one and not github as link.

Because for now the Github link can't work (as explained in the current ticket ;) )

comment:13 Changed 10 years ago by skorasaurus

What information do you still need that I could provide ?

comment:14 Changed 10 years ago by skyper

Owner: changed from skorasaurus to team
Status: needinfonew

Changed 9 years ago by dommage

Attachment: patch_8686_1.diff added

Patch to allow more lenient searching of style zip files for icons.

comment:15 Changed 9 years ago by dommage

Patched ImageProvider.getIfAvailableZip with code from MirroredInputStream.getZipEntry to allow a more lenient search for resources in style zipfiles. This is consistent with the way the the .mapcss file is found within the zipfile, but that leniency was not being extended to the icon images.

While this does increase the cost of looking up image files, it is only for zipped style resources and they are subsequently cached.

NOTE: I considered capturing the zipfile path prefix of the .mapcss file and propagating it to the ImageProvider, but it would have entailed much greater changes for what should be a simple request.

comment:16 Changed 9 years ago by dommage

Summary: Allow ZIP files whose paths are relative to the ZIP root in JOSM's map styling[PATCH] Allow ZIP files whose paths are relative to the ZIP root in JOSM's map styling

comment:17 Changed 9 years ago by stoecker

With your patch an "hello.png" would also find "Myhello.png"? Don't sounds right to me.

Changed 9 years ago by dommage

Attachment: patch_8686_2.diff added

Updated patch that tries exact path match first.

comment:18 Changed 9 years ago by dommage

True enough. I have added the exact match check back in (the old behaviour). That should (overall) improve performance in the general case and still help when the images are not hanging off the root.
In the case where the same file name or file ending exists in two different sub-folders there is no issue, the key for each will include the folder name.

1) hello.png
2) my_hello.png
3) foo/hello.png
4) foo/my_hello.png
5) bar/hello.png

With this change number 4 could still cause problems, but only in zipfiles that suffer from the original 'nested-root' issue, not in existing zipfiles.

comment:19 Changed 9 years ago by AlfonZ

I've tried

test/test.mapcss

node[name=foo] { icon-image:"foo.png"; text:auto; }
node[name=barfoo] { icon-image:"barfoo.png"; text:auto; }

test/foo.png
test/barfoo.png

and it used barfoo.png for both nodes

comment:20 in reply to:  15 Changed 9 years ago by bastiK

Replying to dommage:

Patched ImageProvider.getIfAvailableZip with code from MirroredInputStream.getZipEntry to allow a more lenient search for resources in style zipfiles. This is consistent with the way the the .mapcss file is found within the zipfile, but that leniency was not being extended to the icon images.

While this does increase the cost of looking up image files, it is only for zipped style resources and they are subsequently cached.

NOTE: I considered capturing the zipfile path prefix of the .mapcss file and propagating it to the ImageProvider, but it would have entailed much greater changes for what should be a simple request.

I think the endsWith approach is a bit too pragmatic. What greater changes are we talking about?

Changed 9 years ago by dommage

Attachment: patch_8686_3.diff added

Prevent matching files with the same postfix.

comment:21 Changed 9 years ago by dommage

The only reason the style is processed at all is that the MirroredInputStream.getZipEntry goes hunting for the .mapcss in much the same way. That is the only place that JOSM is currently aware that the style is not located in the root of the .zip file.

I ran through a few possibilities before settling on the endsWith (which I agree is a bit simplistic).

  • 'correcting' the paths in the mapcss post-parsing
  • 'correcting' the zip structure on the fly (by stripping any directories above the .mapcss file)
  • making the zip lookup (ImageProvider.getIfAvailableZip) 'smarter'

Unfortunately, all of these require the app to be aware that there is a .zip structure/.mapcss path mismatch; Information that currently only exists in MirroredInputStream.
There is an added problem in that this particular style file also includes references to builtin images which would have to be left unchanged. (The v2 diff manages that by putting back the exact match check.)

I've added one last attempt using endsWith as an attachment (patch_8686_3.diff). It relies on the initial getEntry to keep everything that already works working. It then makes the assumption that it is looking for an entry in a sub-directory and uses a '/' to make the name match exact.

comment:22 Changed 9 years ago by AlfonZ

Another edge case:

test1/foo.png
test2/test.mapcss
test2/foo.png

uses test1/foo.png instead of intended test2/foo.png.
Although I admit it's unlikely to happen.

Changed 9 years ago by dommage

Attachment: patch_8686_4.diff added

If entry not found, search relative to the mapcss file location.

comment:23 Changed 9 years ago by dommage

If the entry is not found with the given full_name, find the mapcss file and use it's location as the root of the requested images.

comment:24 Changed 9 years ago by stoecker

Hmm, these are all workarounds which I don't like. Fixing the system by properly passing the path seems much better to me.

comment:25 in reply to:  23 ; Changed 9 years ago by bastiK

Replying to dommage:

If the entry is not found with the given full_name, find the mapcss file and use it's location as the root of the requested images.

Patch 4 goes in the right direction, but MapCSSStyleSource and MapImage should be aware of the path of the corresponding .mapcss file inside the zip, so ImageProvider can be called with the right parameters directly.

comment:26 in reply to:  25 Changed 9 years ago by dommage

Replying to bastiK:

Patch 4 goes in the right direction, but MapCSSStyleSource and MapImage should be aware of the path of the corresponding .mapcss file inside the zip, so ImageProvider can be called with the right parameters directly.

I don't disagree, but I have reached the limits of my comfort with the codebase. The patch as given solves the users issue without (so far as I can tell) breaking any existing use cases. They've waited three months, perhaps we could release this and open a new ticket to investigate enhanced mapcss handling?

comment:27 Changed 9 years ago by bastiK

Resolution: fixed
Status: newclosed

In 6148/josm:

fixed #8686 - Allow ZIP files whose paths are relative to the ZIP root in JOSM's map styling

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.

Add Comment


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

 
Note: See TracTickets for help on using tickets.