openhab / openhab-android

openHAB client for Android
https://play.google.com/store/apps/details?id=org.openhab.habdroid
Eclipse Public License 2.0
598 stars 317 forks source link

Add support for external fonts and CSS files in SVG images #2022

Open ewaldc opened 4 years ago

ewaldc commented 4 years ago

Is your feature request related to a problem? Please describe.

Problem: Rapidly change the style for a whole collection of SVG images/icons by having a shared CSS file that is included in each SVG (e.g. change color, dark/light mode switch).

The included SVG rendering library AndroidSVG supports external fonts and CSS files (e.g. SVG @import "style.css") but the user application should override/implement the SVGExternalFileResolver class. Documentation is here and in official SVG pages

Describe the solution you'd like

Extend SVGExternalFileResolver with resolveFont and resolveCSSStyleSheet e.g. in ExtensionFuncs.kt. Happy to give it a shot (although I am new to Kotlin) and if it works submit a PR

Describe alternatives you've considered

Include all styling in each SVG (versus a shared CSS file of all SVG).

Additional context

Add any other context or screenshots about the feature request here.

Chart with imported CSS and one without (for testing) chart_extCSS.zip chart.zip

PS. These SVG charts also don't render properly on the AndoridSVG library, but that is an issue with that library and I'm filing a PR with them (or find a workaround)

maniac103 commented 4 years ago

Happy to give it a shot (although I am new to Kotlin) and if it works submit a PR

:+1: Go for it!

Some implementation notes:

mueller-ma commented 4 years ago

Some thoughts on this:

The included SVG rendering library AndroidSVG supports external fonts and CSS files (e.g. SVG @import "style.css")

IMO you should open an issue in the https://github.com/openhab/openhab-webui repo as well to agree on a path for the imported files.

Rapidly change the style for a whole collection of SVG images/icons

IMO for icons there a better ways to quickly change the whole set:

ewaldc commented 4 years ago

The source CSS and font files are simply put into the images directory on the server?

Yes, CSS would typically be in same folder (or subfolder) as SVG. With respect to the 3 external types of resources:

IMO for icons there a better ways to quickly change the whole set: Rename the folder

Renaming the directory (icons-light/icons-dark to icons) is what I currently do, but it still requires every SVG to be edited individually (for fill colors etc.) and then duplicated. The idea was to be able to just rename the CSS file instead (theme-light.css/theme-dark.css --> theme.css) if @import would be implemented since I found that the HabDroid app already passes the theme (e.g. theme = dark/light) e.g. as part of the URL arguments when it request a chart (which is really great!)

Since we can simply extend the resolver class, we should do so instead of using extension functions. The class constructor likely will need a reference to an HttpClient instance to fetch the images from

Exactly what I had in mind :-)

There's a huge caveat due to bad SVG library design: we only can have a resolver singleton, but we have two possible HTTP clients (active and remote server) ... I'm not sure how to solve this in a non-hacky way. A proper library design would allow passing in a given resolver when calling the SVG factory methods.

Technically the SVG spec allows for a full URL path name (e.g. to a foreign server) but that would introduce all kind of issues (e.g CORS) and that is also way beyond the use case. So I had in mind to limit the @import to the same server that is providing the SVG (pick up that connection and generate a HTTPClient for it, if somehow possible).

maniac103 commented 4 years ago

pick up that connection and generate a HTTPClient for it, if somehow possible

That's the problem. One could have two threads loading SVGs simultaneously, e.g. when a notification with SVG icon arrives (loaded from remote connection, usually myopenhab) while loading sitemap data (loaded from active connection, which may be local or remote depending on active network type) ... the problem is that in the resolver, you have no reference to the caller which triggers the request.

ewaldc commented 4 years ago

pick up that connection and generate a HTTPClient for it, if somehow possible

With the attached patch I seem to be able to pick up the url for all bitmaps from the appropriate connection. It hits only 3 locations and requires really minor, straightforward changes. It took just 5 minutes and I did not experience or see any bad design IMHO (rather the opposite)...

From there I assume I can now create a HTTPClient using httpClient.buildUrl(url)

mypatch.zip

maniac103 commented 4 years ago

From there I assume I can now create a HTTPClient using httpClient.buildUrl(url)

No. You can ask an existing HttpClient object to convert a (potentially) relative URL to an absolute URL by calling that method on it. In your case, you don't have a HttpClient object reference, which is the problem I mentioned. It also doesn't help to pass the client reference to InputStream.svgToBitmap, since the resolver is a singleton without any reference to the SVG.getFromInputStream() call, and multiple calls to InputStream.svgToBitmap can happen in parallel.

ewaldc commented 4 years ago

Ah, excuse my slow brain. Covid19 infection has done little to improve it. Back to square one...

ewaldc commented 4 years ago

This patch is getting me a HTTPClient, and the baseURL seems OK.

Basically it just extends class HttpResult with the HTTPClient, so it can be passed it on.

Probably the changes in ItemUpdateWidget.kt (fetchAndSetIcon) are not correct (might not need a new connection from the factory), but I was unable to trigger (a breakpoint in) that function to see if I could solve it in a simpler fashion. Don't know how to trigger that code...

mypatch.zip

Sorry if I'm testing your patience...

maniac103 commented 4 years ago

This patch is getting me a HTTPClient, and the baseURL seems OK. Basically it just extends class HttpResult with the HTTPClient, so it can be passed it on.

So now you got the HttpClient in the SVG conversion routine ... but we need it in the resolver class instance. How do you get it there? ;-) Creating a new resolver instance every time (with the client as constructor parameter) doesn't work as mentioned before due to the static setter...

Sorry if I'm testing your patience...

You don't, don't worry :-)

mueller-ma commented 4 years ago

@ewaldc Can you upload the code to a new branch in your fork and create a draft PR?

ewaldc commented 4 years ago

This compiles correctly and register SVGExternalFileResolver properly. If I understood correctly, the init function of an object is executed only once and the debugger seems to confirm this.

But the @import does not hit breakpoint. Could be an issue with SVGAndroid or my limited Kotlin knowledge...

private val svgResolver: SVGExternalFileResolver = object : SVGExternalFileResolver() {
    override fun resolveFont(fontFamily: String, fontWeight: Int, fontStyle: String?): Typeface? {
        return null
    }
    override fun resolveImage(filename: String?): Bitmap? {
        return try {
            null
        } catch (e1: IOException) {
            null
        }
    }
    override fun resolveCSSStyleSheet(url: String?): String? {
        return try {   // --> breakpoint here
            null
        } catch (e1: IOException) {
            null
        }
    }
    init {
        SVG.registerExternalFileResolver(this)
    }
}
maniac103 commented 4 years ago

This compiles correctly and register SVGExternalFileResolver properly.

Ack, but how do you get the HttpClient for a specific SVG-to-bitmap transformation into that instance? ;-)

As for the breakpoint, the try-catch might become optimized away. I'd try a log statement there.

ewaldc commented 4 years ago

One more try:

open class SVGClient {
    var client: HttpClient? = null
    companion object : SVGExternalFileResolver() {
        init {
            SVG.registerExternalFileResolver(this)
        }
        var client: HttpClient? = null
        override fun resolveFont(fontFamily: String, fontWeight: Int, fontStyle: String?): Typeface? {
            return null
        }
        override fun resolveImage(filename: String?): Bitmap? {
            return try {
                null
            } catch (e1: IOException) {
                null
            }
        }
        override fun resolveCSSStyleSheet(url: String?): String? {
            return try {
                Log.d(Util.TAG, "SVG bitmapURL: $client") // --> breakpoint here
                null
            } catch (e1: IOException) {
                null
            }
        }
    }
}
private val svgResolver = object : SVGClient() {}

Now I can set the HTTPClient in fun InputStream.svgToBitmap

        svgResolver.client = client
        val svg = SVG.getFromInputStream(this)

But still no breakpoint being hit. Had to file a defect in SVGAndroid as it does not render fill-opacity properly, so I'll probably need to see where it goes wrong in the code...

maniac103 commented 4 years ago

Now I can set the HTTPClient in fun InputStream.svgToBitmap

See, that's the ugly part that I do not want. Assume you have two conversions running simultaneously, client will be that of the second conversion and (incorrectly) be used also by callbacks for the first conversion. I'll open an issue against the lib, I guess.

maniac103 commented 4 years ago

https://github.com/BigBadaboom/androidsvg/issues/199

ewaldc commented 4 years ago

Tried several solutions but could not find a good way to get around the multi-thread issue besides using a shared mutex around these two statements. The callback should be made context aware as you suggested. Even in good old Linux C kernel all callbacks have context (aka void *). Learned quite a bit of Kotlin today :-).

ewaldc commented 4 years ago

In the meantime, I found out why @import was not triggering the callback. There is a defect in the CSS parser. I will file an issue report (and perhaps a PR as I found where the error is)

ewaldc commented 4 years ago

Can't see how to fix BigBadaboom/androidsvg#199 with a totally static library. Will require the instantiation of a SVG object IMHO to store the context (or unique ID of some kind) otherwise it will be needed to pass that context to all parser functions as the callback happens deep in the parser... In that case one can also register the callback for each instance, which means the API does not need to change. Openhab-android (I'm learning!) can then manage a list/pool of objects containing matching SVG object, HTTPClient and callback handler.

From fun InputStream.svgToBitmap we might then request the svg object that matches the current client. I noticed though that there might be multiple threads (ConnectionFactory?) for the same server (do page refreshes happen asynchronously?) so might to manage a pool (2?) of those objects per OH server (= different baseURL in HTTPClient?) .

val svg = svgResolver.getSVG(client)  // Obtain SVG object from pool
...
svgResolver.setSVG(null) // release SVG object from pool

Filed a defect for @import issue

maniac103 commented 4 years ago

Can't see how to fix BigBadaboom/androidsvg#199 with a totally static library. Will require the instantiation of a SVG object IMHO to store the context (or unique ID of some kind) otherwise it will be needed to pass that context to all parser functions as the callback happens deep in the parser...

Yes, it'll be needed to add this to all parser methods, or alternatively, make parser (or its interface) public and allow setting the resolver on that. Setting the resolver on the SVG instance only would help for animated SVGs (I guess?), not for stuff needed during parsing. IMHO setting it on the parser and inheriting it to the SVG instance from there makes most sense to me.

Openhab-android (I'm learning!) can then manage a list/pool of objects containing matching SVG object, HTTPClient and callback handler.

The SVG object essentially equals the image, I don't think we want to track another list of images used throughout the app (we already have the bitmap cache).

From fun InputStream.svgToBitmap we might then request the svg object that matches the current client.

There might (and likely will) be more than one SVG image per client.

I noticed though that there might be multiple threads (ConnectionFactory?)

ConnectionFactory isn't related to that. The threads come from Kotlin coroutines ... check for withContext(Dispatchers.IO) in HttpClient, everything in those blocks will execute on a separate thread pool.

for the same server (do page refreshes happen asynchronously?)

An incoming SSE event ultimately triggers e.g. an image refresh, which happens asynchronously, yes.

so might to manage a pool (2?) of those objects per OH server (= different baseURL in HTTPClient?) .

You lost me ... what do you mean here?

ewaldc commented 4 years ago

I now have my own fork of androidsvc published in jitpack.io so I can fix defects and test with OH-android build. Already fixed the two issues I submitted... At least I can now test things before submitting a PR. Maybe implement BigBadaboom/androidsvg#199 tomorrow...

ewaldc commented 4 years ago

Finally, it's working but since it requires a (minor) fix to androidsvg, I can't just provide a PR for just OH-android only. It's running here I had to extend the HttpClient with a synchronousGet since one can not suspend and override a Java class that was not designed as "suspendable", but it's reasonably clean except for not being thread-safe. All the colors for the labels are in "chart.css".

chart.zip

Screenshot_20200528-134232_openHAB_Beta

maniac103 commented 4 years ago

I had to extend the HttpClient with a synchronousGet since one can not suspend and override a Java class that was not designed as "suspendable", but it's reasonably clean except for not being thread-safe.

One actually can ;-) Just enclose the call in an runBlocking block.

ewaldc commented 4 years ago

Implemented all optimizations you suggested and fixed indentation. runBlocking works as a charm (learning all the time!). It's a much smaller and cleaner change now:

Could runBlocking also be used to protect the two lines

        SVGClient.client = client
        val svg = SVG.getFromInputStream(this)

from potential client mismatch in case both local and remote connection threads are active (as a temporary solution until #199 is solved) ?

The one thing I still haven't figured out (but also not really investigated) is why in the demo the weather chart is requested (and thus painted) two times. First I thought that this was due to a page refresh while I was single stepping through the debugger, but it happens also in regular run mode. I happens in the regular beta client too, so it's not something caused by the changes.

URI: /chart - METHOD: HTTP_GET - ARGUMENTS: groups = Weather_Chart; dpi = 640; period = h; random = 1570696976; theme = dark; w = 1440; h = 720;

EDIT: main threads 2 and 3 are both processing the SVG in parallel

Working on fixing the two remaining issues of androidsvg (during lunch time as I resumed work now after Covid infection)

maniac103 commented 4 years ago

@ewaldc Can you give this branch a try? It uses this branch of androidsvg. If it works, I'll submit the latter as PR to androidsvg.

ewaldc commented 4 years ago

I think it's missing the fix for resolving external CSS files? Without this fix AndroidSVG will never even call the (now modified) resolver :-)

maniac103 commented 4 years ago

Well, yes, since I don't want to mix changes for separate PRs and that fix deserves a separate commit.

I've pushed a new 'test' branch including your commit to my androidsvg repo and switched the 'svg-css' branch of the app repo to that androidsvg branch.

ewaldc commented 4 years ago

Understood, the only challenge is that without this change, SVG Android will not invoke externalFileResolver.resolveCSSStyleSheet(file); since the mediaList will be of size zero. What I can try is to change my copy of your OH Android to invoke the SVG parser so that a mediaLists exists and then change the SVG chart so that it contains a mediaList that matches with the one in the code...

There is one thing I don't understand: mobile/build.gradle contains compile 'com.github.BigBadaboom:androidsvg:418cf676849b200cacf3465478079f39709fe5b1' Will that result in your fork of SVGAndroid ? I'm not so good with Gradle, always when I think I understand it a little it does something to throw me off again...

maniac103 commented 4 years ago

There is one thing I don't understand: mobile/build.gradle contains compile 'com.github.BigBadaboom:androidsvg:418cf676849b200cacf3465478079f39709fe5b1' Will that result in your fork of SVGAndroid ?

No, that one will use upstream. That's why I changed that in my branch. The SHA1 referenced there (8dc61c0...) is the same as in the test branch in my androidsvg fork

ewaldc commented 4 years ago

Oops, didn't notice Android Studio reset the branch to Master after a crash. Did not notice :-( It's OK now!

ewaldc commented 4 years ago

Congratulations, working to perfection !

There is of course still the Android SVG defect with fill-opacity, but include of CSS works fine. The one thing I still don't understand is why it loads the chart (and now also the css file) twice. This happens with both a jpg and an svg chart. I noticed it first in the logcat when I added a log line where the CSS external resolver was called. I'm running the standard OH2 demo, and tested with both chart.jpg and chart.svg.

Following Logcats are taken from the standard OH2 demo when the user enters the "Outside Temperature" page with pageid 0100: Logcat with official OH22.5.5 --> png charts Logcat with my OH2 server for ESP --> svg charts

It only happens for the chart image, not for the icons. The other interesting observation is that the chart is first requested with avoidCache true and immediately thereafter with avoidCache false but it seems that the HttpClient is ignoring this and fetches the chart image twice anyway:

2020-06-03 09:36:16.414 2272-2272/org.openhab.habdroid.beta I/WidgetImageView: Refreshing image at http://192.168.1.20:8080/chart?groups=Weather_Chart&dpi=560&period=h&random=1421411947&theme=dark&w=1440&h=720, avoidCache true
2020-06-03 09:36:16.414 2272-2272/org.openhab.habdroid.beta I/WidgetImageView: Refreshing image at http://192.168.1.20:8080/chart?groups=Weather_Chart&dpi=560&period=h&random=1421411947&theme=dark&w=1440&h=720, avoidCache false

Will test the image tomorrow and see what can be done about the fonts as I don't understand why SVG Android somehow does not use the default Android Roboto font. Could not find it directly in the code...

ewaldc commented 4 years ago

Font problem is resolved. SVG Android code does not convert font names to lowercase so font name "Sans-serif" ends up being not found. When that happens, SVG android does not use the "sans-serif" (Roboto) default Android font but uses "serif". It also does not translate "roboto" to "sans-serif". I changed the font in my chart to "sans-serif" and all is fine now, so there is no need to load external fonts. Anyhow the code would be a bit more complex as it's a two step process IMHO (download font in background, load typeface from file).

mueller-ma commented 4 years ago

HttpClient is ignoring this and fetches the chart image twice anyway

Can you enable debug logging to see if the chart is really fetched twice from the server? Go to the openHAB app settings => "Show information for troubleshooting".

I changed the font in my chart to "sans-serif" and all is fine now

Did you open an issue in the SVG Android repo?

It also does not translate "roboto" to "sans-serif".

AFAIK you always secify more than one font: At least one font family (e.g. roboto) and one generic family as fallback (e.g sans-serif).

https://www.w3schools.com/Css/css_font.asp

ewaldc commented 4 years ago

Can you enable debug logging to see if the chart is really fetched twice from the server? Go to the openHAB app settings => "Show information for troubleshooting"

I have, see attached logcat above. I also verified on the OH2.5.5. Jetty server as well as my OH2 for ESP server and indeed: it is requested (and delivered) twice. The OH Android logs also show that both requests are not exactly the same: one is with caching enabled, the other one not ! And when using the debugger, I can see it's done in two parallel running threads (started from WidgetImageView?) , running side by side, to the point where I hit every breakpoint twice...

Did you open an issue in the SVG Android repo?

I have not:

I created a chart in SVG with a nice OpenHab Logo (as external image) in it which I will use to test resolveImage() tomorrow. BTW. Very nice how you implemented the thread-safe SVGFileResolver: clean and simple! It's also much cleaner to have a separate Kotlin file to define the SVGFileResolver class (why did I not think of that :-( ). Have learned a lot over the last weeks!

maniac103 commented 4 years ago

And when using the debugger, I can see it's done in two parallel running threads (started from WidgetImageView?) ,

The threads are a side effect of the coroutine used by the HttpResponse.asBitmap method.

running side by side, to the point where I hit every breakpoint twice...

Putting a breakpoint here it would be interesting from where the two calls to this method are triggered.

ewaldc commented 4 years ago

After removal of this line loading of an image works great. Notice the OH Logo and the fonts that now match the OH Android fonts. For testing it would be easier if this line of code would not hard-code the images to the /images directory. This way one can test the SVG file with a browser and not have to maintain the image file in two locations. In the SVG file the image would then point to "/images/" and resolve properly for OH Android, OH servers as well as web browsers.

httpClient.get("$filename").asBitmap(0, false).response

Screenshot_20200604-121809_openHAB_Beta svg.zip

ewaldc commented 4 years ago

Putting a breakpoint here it would be interesting from where the two calls to this method are triggered.

Here are the results:

Haven't done any further digging yet...

maniac103 commented 4 years ago

OK, those calls are kinda to be expected. What's weird those is the first actually triggering execute(): that code path is meant for finishing requests that were interrupted due to UI rebuild after e.g. a device rotation. If the page is first opened I would not expect the image view having even seen a request, let alone a request that hasn't completed... At that first call point, the values of url and job members would be interesting specifically the state the job is in.

I'll also see to have a look into it when I find some time.

ewaldc commented 4 years ago

The other thing that seems odd wrt the second invocation is this test

The image is loading at that moment, so why is this test not true? It seems there explicitly to avoid a double load ...

ewaldc commented 4 years ago

Ah, found it. First url is:

http://192.168.1.32:8080/chart?groups=Weather_Chart&dpi=560&period=h&random=2001073041&theme=dark&legend=true&w=1440&h=2280

Second url is:

http://192.168.1.32:8080/chart?groups=Weather_Chart&dpi=560&period=h&random=1528545428&theme=dark&legend=true&w=1440&h=2280

There is a random element in the chart that makes each load url unique. Hence the code that should prevent double loading fails...

maniac103 commented 4 years ago

Makes sense, thanks for that investigation. I'll think about a solution.

maniac103 commented 4 years ago

OK, checked this again. What should be happening when opening a new page with a chart item is:

So while execute() is called 2 (no refresh) / 3 (refresh) times, the image should be actually requested from the server only once (no refresh) / twice (refresh). @ewaldc Is refresh enabled in your case?

ewaldc commented 4 years ago

Indeed that is the sequence I am seeing in the debugger/no debug + added log code

I see this behavior also on (some) icons, but in the case of these icons this test is preventing that a refresh would cause a second image server request while that same icon is still rendering.

However in the case of a chart image, every invocation/URL has a unique random number in it, causing this prevention code to fail as it simply compares the last requested image url with teh current one. At least that is what I see with my limited knowledge (and adding a few log lines to validate this and eliminate effects causes by debugger) ...

maniac103 commented 4 years ago

Oh, so we were talking about ChartActivity all this time? I thought were talking about widget pages... But anyway, the refresh thing is the same as I mentioned earlier, in #1717 @mueller-ma wanted to reload the image when bringing back the app from background, but the code introduced there also is what triggers this double fetch. I'll look into that; I already have an idea how to fix it. Basic idea is not forcing a refresh in onStart or onResume, but just clearing the last refresh timestamp in onPause. That way the double fetch is avoided, but when coming back from background the image is fetched again, since the next refresh is due in the past in that case.

ewaldc commented 4 years ago

Oh, so we were talking about ChartActivity all this time?

Not really. It is about the number of (non-cached) server requests to an image object (SVG or other) when loading a page. I often see this 3x invocation execute() when loading a page, but only in the case of a chart is results in a double GET.
To be able to trigger page loads I went back and forth between page id 0100 and tapping on the chart which brings up a page with just the chart. I assume this why my stack track had ChartActivity() in it as it was probably taken from the page with just the chart.

But on every page load that contains a chart, I see exactly the same double chart paint. Just what's in the stack may not always be 100% the same (I assume...)

maniac103 commented 4 years ago

I assume this why my stack track had ChartActivity() in it as it was probably taken from the page with just the chart.

Yes. Please keep in mind that a normal widget page (WidgetListFragment inside of MainActivity) and the full screen chart view (ChartActivity) are completely different code paths, so when analyzing why images are loaded too often, we need to look at them separately.

ewaldc commented 4 years ago

I figured out so much, and that is why I verified also without the debugger and checked on the server as well. Very sure that both on page 0100 and the chart page solo, the chart images are loaded twice, both with my OH server for ESP as well as official one. Will get stack dumps for page 0100 tonight...

maniac103 commented 4 years ago

@ewaldc Please give #2039 a try.

ewaldc commented 4 years ago

Sorry for the delay, I was on duty the whole weekend (voluntary fire fighter). For page 0100 ("Outside temperature") it's perfect. The chart is never painted twice. For the weather_chart page (the one you access when tapping the chart) it draws the chart twice when you come from the main page --> page 0100 --> weather_chart page, but not when I go back and forth between page 0100 ("Outside temperature") and the weather-chart page.

Haven't had the time to look into why that is the case.