Opened 4 years ago

Closed 3 years ago

#1550 closed enhancement (fixed)

Suggestion for plugin enhancement

Reported by: cacapo Owned by: cacapo
Priority: trivial Milestone: 0.9.25
Component: apps/plugins Version: 0.9.19
Keywords: plugin Cc:
Parent Tickets:

Description

Suggestion: let plugins register an icon to display on the router console homepage instead of the default jigzaw. Given that the plugin-mechanism is now so inviting with the load-from-file function I think it makes sense. Only small modifications of NavHelper?.java and PluginStarter?.java are required. And of course the introduction of a new property: console-icon.

Subtickets

Attachments (23)

NavHelper.java (3.5 KB) - added by cacapo 4 years ago.
NavHelper? source
PluginStarter.java (41.4 KB) - added by cacapo 4 years ago.
PluginStarter?.java
PluginStarter.diff (663 bytes) - added by cacapo 4 years ago.
NavHelper.diff (854 bytes) - added by cacapo 4 years ago.
PluginStarter.patch (2.0 KB) - added by cacapo 4 years ago.
NavHelper.patch (2.4 KB) - added by cacapo 4 years ago.
PluginStarter.2.patch (2.0 KB) - added by cacapo 4 years ago.
In correct file order (yeah I know........)
NavHelper.2.patch (2.4 KB) - added by cacapo 4 years ago.
In correct file order
NavHelper.alt.patch (2.5 KB) - added by cacapo 4 years ago.
PluginStarter.alt.patch (2.2 KB) - added by cacapo 4 years ago.
PluginStarter.alt2.patch (2.9 KB) - added by cacapo 4 years ago.
icon2prop.sh (422 bytes) - added by cacapo 4 years ago.
PluginStarter.alt3.patch (3.6 KB) - added by cacapo 4 years ago.
icon2prop.alt3.sh (797 bytes) - added by cacapo 4 years ago.
servletpatch.patch (7.9 KB) - added by cacapo 4 years ago.
Solution with servlet - on the fly generation
PluginIcons.patch (8.9 KB) - added by cacapo 4 years ago.
CodedIcon.patch (5.4 KB) - added by cacapo 4 years ago.
cacapo_coded_icon.patch (4.8 KB) - added by cacapo 3 years ago.
Icon2proP.sh (428 bytes) - added by cacapo 3 years ago.
Script to convert .png to B64 property
orchid_coded_icon.patch (3.5 KB) - added by cacapo 3 years ago.
Patch to modify plugin.config in the Orchid plugin
review_patch.patch (7.0 KB) - added by cacapo 3 years ago.
Servlet solution for review
2_review.patch (7.7 KB) - added by cacapo 3 years ago.
Attempt to fix the 4 points above
www_patch.patch (1.1 KB) - added by cacapo 3 years ago.
Suggestion for plugin specs

Download all attachments as: .zip

Change History (52)

Changed 4 years ago by cacapo

NavHelper? source

Changed 4 years ago by cacapo

comment:1 follow-up: Changed 4 years ago by zzz

would you please attach a diff or patch file for easier review, thanks

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

comment:2 Changed 4 years ago by cacapo

Like this???? (I'm new to this)

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

comment:3 in reply to: ↑ 1 Changed 4 years ago by cacapo

Replying to zzz:

would you please attach a diff or patch file for easier review, thanks

Patchfiles, slightly more presumptous than the diff files added. The code commented out could be removed I think. It compiles and runs in my local branch anyway. Although I haven't the Bote plugin installed. If that dev could be swayed to include an icon, the hardcoded hack could go as well.

Changed 4 years ago by cacapo

In correct file order (yeah I know........)

Changed 4 years ago by cacapo

In correct file order

comment:4 Changed 4 years ago by zzz

  • Owner set to zzz
  • Status changed from new to accepted

thanks, will review in the next week or so

comment:5 Changed 4 years ago by zzz

The way you've done it here (PluginStarter:294) :

        iconfile = (iconfile == null) ? "/themes/console/images/plugin.png" : appName + "/" + iconfile;

will only work if the plugin is a webapp (war). Can't blame you, it's the easy way.

Almost all plugins have a webapp component. For the others, our options are:

  • too bad, they will get plugin.png
  • put the icon in plugin.config, base64-encoded
  • specify a path to the file relative to the plugins/appName/ directory, then come up with some way for the console to get at it (pita)

what do you think is best?

comment:6 Changed 4 years ago by cacapo

Wow, didn't see that one coming.

Trying to think about the scenarios.

The way it is now they will always get plugin.png NavHelper:84

If we introduce a way not do to that, we have to take height for them not having webresources, meaning they would have to refrain from using the property if they don't, or find a convoluted way to get to it.

I'm thinking it is a very rare scenario where the plugin does not have a webresource. Are they not more likely to be applications written to the streaming library without the router as a vehicle? If so I would say it is better to present the icon-opportunity as an extra benefit of webapplications within the router and stay with the default icon for other cases.

Option 3 feels like it would make a pretty mess with the code. I like it when changes make things lighter and clearer. Option 2 is cool - that one was not even in my cognitive function but perhaps overkill. So I would say option 1 is the most likely to make things better not worse.

But does that mean the plugin has to present a type-identification of sorts?

comment:7 Changed 4 years ago by zzz

Off the top of my head, zzzot, i2pcontrol, i2phex, jwebcache, jircii, neodatis, and seedless don't have a webapp. Just guessing, not 100% sure. So almost half, and some of the more popular ones. Some offer a web server at a different port (not a webapp under the console), some fork off a different process.

The plugin code just looks for a webapp in the standard place, and loads it if there. See the plugin docs.

Now, if options 2 and 3 are too hard or too complex/clever or a bad idea, I'm fine with option 1. Or even that we check in what you've posted now, and offer some alternate solution later, if we get around to it. But let's decide that knowing that it doesn't work for everybody.

comment:8 Changed 4 years ago by cacapo

Ah, I think I finally get it. Anything without 'console/webapps' falls straight through (PluginStarter:359) but is still registered at (PluginStarter:423,425) if it has a link prop. (Took me 2 commutes)

But then option 2 suddenly seems like a really good idea. Can I have a go at it or do we have to close the ticket? I can see there are a few things I have to learn on the way.

comment:9 Changed 4 years ago by zzz

Yes, your analysis is correct.

Let's keep this ticket open.

Do you suggest that I check in the current solution, while you work on option 2 as an alternate method? Or hold off while you explore option 2, with the thought that it may become the only method?

Note that option 2 may or may not be easier or better than option 3, haven't really thought it through, and you may only discover which is better after trying to implement one or the other.

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

comment:10 Changed 4 years ago by cacapo

It's such a trivial feature, but if it is included I think it should allow a web resource to contain the icon as the premier option. I'm thinking that the target group is people coming from web development rather than crypto and then it is a nice perk. On the other hand, a very minor feature mustn't be allowed to clutter down the code.

But two options would be more generous than one I think.

I tried moving the evaluation of the web resource into the block for 'console/webapps' it works just as well there, and then it would be ignored even if present in a config file for a webless plugin.

If I'm not totally wasting your time I'd like to explore both options further. No need to push it.

comment:11 Changed 4 years ago by zzz

  • Component changed from apps/console to apps/plugins
  • Owner changed from zzz to cacapo
  • Status changed from accepted to assigned

"alt" patches above cleaned up and checked in as ab8ab17de32088a74083f5ce7c2aad8862d3f10b 0.9.19-19-rc.

Website docs updated in e23f5bd71e8eae171d5ff6aad3e06cc92897f9f4

Leaving ticket open and reassigning to you for investigation of the additional options.

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

comment:12 Changed 4 years ago by cacapo

Brief suggestion for solution to option 2 (and 3).

I did not use uuencode after all.

  • It has weak support in standard Java library as far as I can tell
  • The format with lots of whitespace and linefeeds will make parsing error-prone

Instead I used the native i2p Base64 encoding. The little script attached will make generation of code easy.

This short code example assumes all files to be of type .png.

The key element is the introduction of a pseudo theme, which makes storing external files possible without violating the integrity of baseDir.

Not heavily tested or proofread, but it works in my local build.

Two plugins from my eepsite:

Last edited 4 years ago by cacapo (previous) (diff)

comment:13 Changed 4 years ago by cacapo

A more realistic version of above. Stipulates that the encoded file string starts with 3-character identification of file format. Supported types are those recognized by viewtheme.jsp i.e. png, ico, gif and jpg. This is done automatically in the shell script.

Last edited 4 years ago by cacapo (previous) (diff)

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

Changed 4 years ago by cacapo

Solution with servlet - on the fly generation

comment:14 Changed 4 years ago by cacapo

And this is the other way of doing it that I can think of. The servlet is unnecessarily chatty because I just lifted it from a NetbeansTemplate?, it would be nicer just extending GenericServlet?. Most of the code in PluginStarter? is now commented out. To be more consistent with the codingstyle a copy of flags.jsp would do the same thing. This is a lot less sneaky than the other method, but also more invasive, adding a new class.

comment:15 Changed 4 years ago by zzz

b64 of a 2KB icon would be 2500 chars which is pretty fugly but the user won't see it, and typical limits for URL length including params is more like 4 KB. But it's pushing the limit.

Perhaps decode the icon once in PluginStarter?, store the binary in a NavHelper? hashmap, and then have the servlet/jsp get it from NavHelper? and output it?

Either way, the on-the-fly method avoids the problem of rewriting the icon to the file system, and then having to clean it up later.

Thanks for putting all files in one patch file attachment, much easier to review

comment:16 Changed 4 years ago by cacapo

Perhaps something like this? We would still have to name the filetype in the header right? Or is that unnecessary? My browser doesn't seem to care what kind of image binary it's pushing regardless of filetype. flags.jsp is particular about it though, so I did this instead: store the undecoded data in a clob in NavHelper? with filetype prepended to the encoded data. This still removes any concerns about URL-size. Presumably the maximum length of a String is 231 .

Last edited 4 years ago by cacapo (previous) (diff)

Changed 4 years ago by cacapo

comment:17 Changed 4 years ago by zzz

By filetype you mean like a Content-Type I assume. Easiest to just require (and assume) png rather than get fancy.

And as I suggested in comment 15, storing in binary as a byte[] in the NavHelper? hashmap is more space-efficient and time-efficient, as it's only decoded from base 64 once. Which you presumably can't do if you are prepending it with a Content-Type.

Yes strings can be long, but I believe our code that reads plugin.config uses DataHelper?.readLine() which has an 8 KB limit, just to prevent trouble.

comment:18 Changed 4 years ago by cacapo

So that would mean something like this?

Changed 4 years ago by cacapo

comment:19 Changed 4 years ago by zzz

yup. super-simple, super-clean.

Changed 3 years ago by cacapo

Changed 3 years ago by cacapo

Script to convert .png to B64 property

Changed 3 years ago by cacapo

Patch to modify plugin.config in the Orchid plugin

comment:20 Changed 3 years ago by cacapo

Patch to let the routerconsole recognize the property icon-code with a .png-file encoded by the script Icon2Prop.sh. Example patch for the Orchid plugin.

comment:21 Changed 3 years ago by cacapo

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:22 Changed 3 years ago by zzz

  • Milestone changed from undecided to 0.9.25
  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for bringing this back to life.

Let's put this on the list for 0.9.25.

Reopening as tickets shouldn't be fixed and closed until the code is in trunk.

In your servlet, you will NPE if the binary isn't found. You should either return 404 or the default image, whatever makes more sense or is easier.

And just to throw an alternative in at this late date: inline image data https://en.wikipedia.org/wiki/Data_URI_scheme ... we'd have to convert from our base64 to standard base64. But then we wouldn't need a servlet at all. Worth thinking about. Not necessarily better.

comment:23 Changed 3 years ago by zzz

also in the servlet, you should set some other headers like cache-control, content-length, last-modified, etc.

You can look in one of the jsps-that-are-really-servlets like flags.jsp for examples

Changed 3 years ago by cacapo

Servlet solution for review

comment:24 Changed 3 years ago by cacapo

This version handles two error-conditions. First if NavHelper?.getBinary() returns a nullpointer, second if the property is present in plugin.config but without attached content. Both cases will redirect to docs/themes/console/images/plugin.png. Caches the icon for a day. Exception handling lifted straight from flags.jsp.

Tested with Firefox, Chromium and Midori

comment:25 Changed 3 years ago by zzz

OK, I tested it, looks really good. Fix these and I'll check it in. I plan to stick it in my i2p.i2p.zzz.test2 branch, which will be propped to trunk in a couple of days.

Also, (not required before I checkin the code), please supply a separate patch to the website plugin spec that documents the new option.

1) No tabs allowed in java files (see http://i2p-projekt.i2p/en/get-involved/guides/dev-guidelines )

2) Please put "} else {" on a single line

3) At PluginStarter? line ~358, you don't need to strip HTML from the icon code (it's not being displayed as-is), just do fullprop = props.getProperty("icon-code")

4) Dies NPE if no "plugin" parameter is supplied (name is null)

 Error 500: /Plugins/pluginicon Server Error

java.lang.NullPointerException
     at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:333)
     at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:988)
     at net.i2p.router.web.NavHelper.getBinary(NavHelper.java:48)
     at net.i2p.router.web.CodedIconRendererServlet.service(CodedIconRendererServlet.java:36)
     at javax.servlet.http.HttpServlet.service(HttpServlet.java:848)
     at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:684)
     at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1496)
     at net.i2p.servlet.filters.XSSFilter.doFilter(XSSFilter.java:28)
     ...

Changed 3 years ago by cacapo

Attempt to fix the 4 points above

comment:26 Changed 3 years ago by cacapo

Read and tried to implement the guidelines as well as fix above!

comment:27 Changed 3 years ago by zzz

Checked in as e1e41e2cf596fa65cd8a009a4953d6165a4b2244 and minor cleanup in ad4de54f23e884bcc360184a1a827f151ece98a8 in i2p.i2p.zzz.test2, to be propped to trunk probably today or tomorrow for 0.9.25. Well done, congrats!

Leaving ticket open until propped and plugin specs on website are updated, please supply website patch at your convenience, no rush, but before 0.9.25 release.

Changed 3 years ago by cacapo

Suggestion for plugin specs

comment:28 Changed 3 years ago by cacapo

OK, that was surprisingly hard to accomplish. And I am very confused about the tabbing in the www docs. Is the wording reasonable though?

comment:29 Changed 3 years ago by zzz

  • Resolution set to fixed
  • Status changed from reopened to closed

lol now I have you all tab-paranoid. It's fine, thanks
www patch pushed to website
test2 was propped to trunk as 0.9.24-2 a couple days ago
that's it!

Note: See TracTickets for help on using tickets.