[WIP] Steps towards compatibility with Android

[Namnodorel]

Soo, I made an attempt at a first step for implementing #49 - created abstract classes for the AWT classes kumo uses, each with just the methods kumo and fields makes use of. Then I replaced the existing AWT usages with using the new abstract classes. I also created a package with the AWT implementations of the abstract classes.
Instantiation of this stuff is now done through a class called InstanceCreator - it has static methods to retrieve objects of the desired kind. Later on, this classes will decide based on its environment which implementation of the abstract classes (the AWT set or the Android set) to supply. The core of kumo should be completely agnostic to what it is working with.

This PR is not ready to be merged. I made it already now so that what I've done so far can be criticized. I don't really have experience with making APIs compatible with different platforms like this, so I'll probably tend to make things unnecessarily complicated. Some things that need to be improved:

• There should be a better name for the abstract classes. An Abst-Suffix doesn't feel descriptive enough to me.
• Although all Unit Tests pass, the output of some of them seems incorrect, especially regarding the image backgrounds sometimes. I'm not sure why this is yet - if everything had went correctly, no functionality should've been changed so far.
• Obviously, both the actual Android implementation and something to decide which implementation to use is still missing.

Feel free to comment on anything about this.

[kennycason]

Thanks, nice work. I'll try and get around and review in more detail. I'll also dig into the API design a bit and see what we can't do to simplify if it makes sense. The general direction makes sense.

In regards to the Abst* names. I felt the same way as well, let me look through it and see if I can come up with some other names. :) Naming is always hard.

[Namnodorel]

The most recent commit fixes the issue with the image backgrounds. I apparently switched height and width somewhere by accident, that was all. But not everything on that front is fixed yet: bubbletext.png only shows symbols for missing characters (rectangles), instead of bubblerized text. The same thing is true for chinese characters and upside-down text. Also, cnn.png is currently void of any words.

Edit: Nevermind, those issues actually already appear when running Unit Tests on the latest master commit. Might be something lokal, I dunno.

[Namnodorel]

The latest commit uses reflection to get new instances of objects. I changed this because AFAIK, the JVM (and thus Androids Dalvik) loads classes only when they need to be accessed. This means that hopefully, the AWT/Android specific implementations will be able to coexist within one package, because their classes will only be accessed (and thus imports tried to be resolved), when kumo runs on their respective platform.

[sirmordred]

Hi mates, i just needed wordcloud on android too, and i made it 👍 here you go https://github.com/sirmordred/WordCloud it is generally in RC state now so you can use it, but still has some alignment/placement issues (android's f*king text glyphs) so any contributions are welcome 👍

EDIT: nevermind, i fixed the problem, lib is now correctly handle all texts regardless they have glyphs or not

[Namnodorel]

@sirmordred Thanks for the note, but unfortunaley at least I need some of the more advanced features that currently only kumo can provide.

[sirmordred]

Well, it fulfills my requirements, thats why i did it, and published it, making kumo compatible with android is really long and complicated way(it will also make kumo more complex) because kumo relies heavily on non-android APIs and so doing separate android version is more easier and maintanable i think, good luck with porting 👍 let me know if you need any help (especially drawing part), kind regards

[Namnodorel]

Finally, I got the Android implementation to work! There's a lot in that commit, so here is a rough breakdown:

InstanceCreator now has a static block that tries to use Class.forName to figure out whether kumo is running on Android. The block decides which implementation package to use.

Also in regards to the abstract classes so far, ColorAbst and ImageAbst now only declare their getters as abstract methods - the attempt to generalize a bit of logic there was misplaced. I've also removed drawAndFinish() from GraphicsAbst, and made the ImageRotation static utility class into another abstract ImageRotatorAbst because having a custom implementation of this for Android was the only way I could make it work reasonably (connected to this, I also moved the conversion between degrees and radiants to the AWT implementation, Android doesn't really work with radians). Obviously the previous code from ImageRotation is now in ImageRotatorImpl of the awtimpl package.

In order to be able to use the Android Framework, I added an additional dependency through a local Maven repo: A small jar with stubs for the framework. It is not bundled with the library, and ignored by InstanceCreator during the integration tests.

Then, of course, there is the big one: the Android implementation of things! Some details took me longer than expected to figure out, but things mostly work fine now. I made a small demo application to test kumo with - if you're interested, I can upload that as a seperate GitHub repo or as a subfolder of the kumo repo. The app contains mostly just slightly modified versions of the kumo integration tests. Here's some screenshots:

The colors are off because Android didn't like the format the colors were given in the current tests, so I set it to random colors just to have something there.

Stuff that still needs to be considered:

• SLF4J (or rather, the standard Java implementation) doesn't work. It does have an Android implementation, but I don't know how or if we can use that yet. SLF4Js way of making use of different bindings basically completely denies choosing the binding dynamically. The only way I see to solve this is to serve a seperate Android build - at which point the whole thing with reflection and figuring out the platform in InstanceCreator becomes pointless.
• Performance. It's fine for smaller, simpler wordclouds. But as the word count gets higher (and more weird angles appear), the performance drastically drops. I ran the Android Studio over the demo app, and it turns out that word collision detection is a huge bottleneck. It's not that the resources of the phone are fully used, instead all the action happens on one thread, and that one takes long to complete. The times may be acceptable on a PC, but on Android, some of the test wordclouds took 20 minutes and more to calculate. That's too much to ask of an average user. Hopefully Performance improvements #80 will improve this, we'll see. Or maybe this could be improved by adding some healthy multithreading?
• Colors. For default colors to work on both platforms, they need to be defined using InstanceCreator(r, g, b) (or argb if that's necessary). Otherwise Android will interpret them as transparent. For the sake of testing, I just put some placeholder values, but those need to be returned to their original color.

[kennycason]

woah! good work man. I recently left my job so will have a lot more time for a bit to work on my open source stuff. I'll be sure to check this out over the weekend/next week.

Performance is definitey going to be a major priority going forward. I honestly didn't expect so much usage of this library early on. :D
I know some people mentioned complexity around Android integration, but I think given the demand, let's try and make it work. I'm confident we can do it and make it simple/clean.

[Namnodorel]

I know some people mentioned complexity around Android integration, but I think given the demand, let's try and make it work. I'm confident we can do it and make it simple/clean.

I think the complexity part gets solved by simply using DI, in this case with InstanceCreator. All of kumos important logic stays the same and can be modified independendly, and the implementations for drawing stuff aren't that big or complicated. What I kinda dislike is the fact that other applications using kumo will probably have to call InstanceCreator as well to use kumo. That doesn't feel elegant enough to me, but I haven't had an idea on how to improve this yet.

Something I just noticed is: Alpha is a little strange in Android (or I'm doing something wrong). Setting the alpha value for the background sets it for the entire image, even when the text drawn on top of the image has alpha 255.

[Namnodorel]

Status update: Since I don't really know if anything can be done about SLF4J on Android and there are no bugs left otherwise (that I know of), I'm now just waiting for either a code review or that #80 will be merged and I can resolve the conflicts it will inevitably cause.

One idea I had to make this PR easier for users of kumo: We could, instead of having the static get-methods, create proxy-classes that do the creation of appropiate objects (the same way the static methods do it now) in their constructor and store the resulting object in a single instance variable. These classes would then also extend the respective abstract classes, and redirect any calls to the object with the actual implementation. I haven't implemented this because even though it makes usage easier for kumo users, it would make the code less easy to read and maintain. If you don't share this concern, I'll happily make the change, but for now I left it like it is.

[kennycason]

I just got back from Hawaii trip and plan to review everything for real this time. Thanks for all the work on this. :)

[Namnodorel]

Soo, I had the idea of just getting rid of the abstract classes altogether and instead use somewhat cleaner interfaces. I have to admit that I kinda got impatient and just wanted to try implementing it - will revert if needed.

This iteration works like this: I created a new package "draw", that contains the platform specific implementations, as well as interfaces for each component. The platform-specific classes extend their respective interface, but so does a platform-independent class for each interface. This platform-independent class just has the simple name ("Color", "Graphics", etc.) and should be usable without knowledge of any of the inner workings by kumo and kumo users. The class just does what I meantioned earlier: It has one instance variable with the actual implementation, and chooses the implementation to use in its constructor. Any calls to this calls are redirected to the actual implementation.

[kennycason]

@Namnodorel Nice. I'll give it another read. I am interested in getting this to a merge point sometime. :) I despite my inactivity, think about this a lot. Someone on my blog mentioned the desire for a GUI to make kumo word clouds which i really wanted to implement, but am holding off until we get this wrapped up. Apologies for my recent absence.

[Namnodorel]

@kennycason Well, this time it was my turn to have other stuff to do - but today, I had time to come back to this and finally implement the performance changes in my branch as well. They do indeed help - the test timings got much better in the normal tests, and still notably better on Android.
Unfortunately, many of the test cases still take minutes to complete in the Android emulator. I feel like this is still more than an average user would be willing to wait for an image to load. After profiling, I found that even with the changes, collision detection is still the biggest bottleneck. I still believe this could be fixed by multithreading at least some of it.

[Namnodorel]

I tested using RxJavas parallel feature, and I can confirm that being able to make use of the full processor capacity speeds things up significantly.
But just slapping RxJava on isn't going to work - while testing, I experienced lots of native crashes that are caused by attempting to make something parallel that wasn't written for it. And I don't think that it's within my abilities to make the required changes :/

Another idea for a performance improvement: The for-loops in WordCloud.place are problematic because they play a big part in the reason that so many collision detections take place. Maybe there could be a way to make the search for space more efficient than just incrementing? Just some sort of smarter placement strategy would be good.

[sparrow007]

Please do something on his pull request because i need to use kumo in my app .

[Namnodorel]

If you desperately want to use it as it is now (which I would not recommend due to the lacking performance on Android), you can use the fork through jitpack.io

[kennycason]

@Namnodorel Yeah, i think ultimately yes, a more efficient placing algorithm is going to be key to performance improvements. I'm still not very sure about all the Android-isms as I just haven't worked with it as much. I still need to review through everything as well on my end. :D

What are your current overall thoughts on merging the branch. You indicated a bit of hesitation as well in last comment. I'm personally in no rush, but of course rebasing could get more difficult for you in time, but I don't have any major work planned for this project yet.

[Namnodorel]

@kennycason I'm not sure what exactly you mean by Android-isms - the core logic stays the same, the part that is different is "just" how images are written to.

I made this because I wanted to use it in a project myself, so I'd like this merged sooner rather than later - but that's easy for me to say because I won't be the one having to deal with the breaking changes that come with this PR. Until we have a better-performing placing algorithm, people will only see the negative of the breaking changes without the positive of usable Android compatibility.

Whatever things you bring up in review I will implement of course, but other than that everything in scope for this PR is done. The only thing different for Android right now is that logging is still missing, but I don't think the way SLF4J works will allow to fix this.

So, I would personally like to merge it, because it makes things easier for me - but I would also understand if you'd rather not deal with the breaking changes until the Android compatibility becomes usable.

[kennycason]

Yeah that makes sense.

So to be clear, the poor placement performance only affects android right? None of the change you made degrade performance for kumo-core as it stands?

[Namnodorel]

On desktop there is a slight performance impact, I'm guessing due to the usage of reflection. The biggest difference I measured was from PolarWordCloudITest, which took 6.5 seconds longer (30s vs 23.5s).

The performance problems that are so problematic on Android aren't present on desktop.