Future ImageParameter refactorings

[matthew16550]

I'd like to talk over my next ideas for simplifying ImageParameter.

1. Add these methods in ImageBuilder then all the diagrams using new ImageParameter(TitledDiagram, FileFormatOption) can use one of them instead. Longer term they might merge into a single method & adding the annotations is decided automatically somehow.

public static ImageData writeStyledImageWithAnnotations(TitledDiagram diagram, TextBlock textBlock, OutputStream os, int index, FileFormatOption fileFormatOption, long seed)
        throws IOException {
    final ISkinParam skinParam = diagram.getSkinParam();
    final StringBounder stringBounder = fileFormatOption.getDefaultStringBounder(skinParam);
    final AnnotatedWorker annotatedWorker = new AnnotatedWorker(diagram, skinParam, stringBounder);
    return writeStyledImage(diagram, annotatedWorker.addAdd(textBlock), os, index, fileFormatOption, seed);
}

public static ImageData writeStyledImage(TitledDiagram diagram, UDrawable drawable, OutputStream os, int index, FileFormatOption fileFormatOption, long seed)
        throws IOException {
    final ImageParameter imageParameter = new ImageParameter(diagram, fileFormatOption);
    final ImageBuilder imageBuilder = ImageBuilder.build(imageParameter);
    imageBuilder.setUDrawable(drawable);
    return imageBuilder.writeImageTOBEMOVED(seed, os);
}

2. Simplify or remove the index param. I haven't seen where the value is actually used?

3. Use diagram.seed() in writeStyledImage() and remove the seed param. We already pass in the value from diagram.seed() in most places except for GitDiagram, JsonDiagram & NwDiagram which use 0 but I assume are safe to use diagram.seed()? Other diagrams that do not use the above new methods are passing in a mixture of diagram.seed(), 42 and 0. I think they can be simplified too but would like to leave them to a later PR when the earlier seed stuff has been removed so it's easier to see what is happening.

4. Looking at the other big ImageParameter constructor, it's used by Help, PSystemColors etc where the user cannot change the skin and the param values are mostly the same for all of them. So we could extract a 3rd static method in ImageBuilder something like writeImageWithDefaultStyle(UDrawable drawable, OutputStream os, FileFormatOption fileFormatOption) plus maybe a few params for values that are not constant.

5. At this point the only place ImageParameter is used will be inside ImageBuilder so perhaps the fields of ImageParameter could be moved to ImageBuilder and ImageParameter can be deleted?

6. Then writeImageTOBEMOVED() should be simple to remove, its used from a few places in ImageBuilder and a few places outside that will need small adjustments first.

Thoughts?

[arnaudroques]

I'd like to talk over my next ideas for simplifying ImageParameter.

1. Add these methods in ImageBuilder (...)

Good point! +1

2. Simplify or remove the index param. I haven't seen where the value is actually used?

Sometimes a single text diagram generates several images. Example:

@startuml
Alice->Bob: Hello1
newpage
Alice->Bob: Hello2
@enduml

index is used to provide which images you want to generate. This is related to Diagram.getNbImages(). So we need somehow this feature. However, maybe we can manage differently this several-images-per-diagram feature, but I think this is another refactoring :-)

3. Use diagram.seed() in writeStyledImage() and remove the seed param. We already pass in the value from diagram.seed() in most places except for GitDiagram, JsonDiagram & NwDiagram which use 0 but I assume are safe to use diagram.seed()? Other diagrams that do not use the above new methods are passing in a mixture of diagram.seed(), 42 and 0. I think they can be simplified too but would like to leave them to a later PR when the earlier seed stuff has been removed so it's easier to see what is happening.

For the record, the only use of this seed stuff is in SvgGraphics. (Well, I think :-) ) It allows to have several SVG images in a single HTML pages without name clashes between SVG ID. Maybe we can deal this in a different way, but this should probably be yet another refactoring :-)

4. Looking at the other big ImageParameter constructor, it's used by Help, PSystemColors etc where the user cannot change the skin and the param values are mostly the same for all of them. So we could extract a 3rd static method in ImageBuilder something like writeImageWithDefaultStyle(UDrawable drawable, OutputStream os, FileFormatOption fileFormatOption) plus maybe a few params for values that are not constant.

Agreed. +1

5. At this point the only place ImageParameter is used will be inside ImageBuilder so perhaps the fields of ImageParameter could be moved to ImageBuilder and ImageParameter can be deleted?
6. Then writeImageTOBEMOVED() should be simple to remove, its used from a few places in ImageBuilder and a few places outside that will need small adjustments first.

Ok again. Since I am very conservative, I suggest that point 5&6 would be implemented in a separate PR.

So maybe you can start by:

• Points 1 and 4. So I can check on my side that I don't see any regression.
• Then another PR points 5 and 6

Point 2 and point 3 should be IHMO two separate PR.

Does it sound good ?

[matthew16550]

Agreed, many little PRs here. Small steps so it's as obvious as possible not changing functionality.

[matthew16550]

Making good progress here. The below diagrams do not include metadata but the code will be cleaner if they all do, will it be ok to have metadata in these also?

• Help
• PSystemCreole
• PSystemCute
• PSystemDefinition
• PSystemOpenIconic
• PSystemTree

PSystemCute & PSystemTree are moot I guess as they are not enabled.

[arnaudroques]

Making good progress here. The below diagrams do not include metadata but the code will be cleaner if they all do, will it be ok to have metadata in these also?

Yes, sure!

PSystemCute & PSystemTree are moot I guess as they are not enabled.

This is stuff I should probably remove now. I'll think about it

[matthew16550]

Would it make sense to merge UGraphicUtils.writeImage() into ImageBuilder? It's being used by FlowDiagram and a couple of error messages.

[arnaudroques]

Would it make sense to merge UGraphicUtils.writeImage() into ImageBuilder? It's being used by FlowDiagram and a couple of error messages.

Sure, yes.

[matthew16550]

Do you have a thorough set of test cases for Flow diagrams? There are many subtle differences between UGraphicUtils.writeImage() and ImageBuilder so I might be missing something.

My basic test case is identical:

@startflow
foo "flow"
@endflow

[arnaudroques]

Actually, Flow is an old try that we should removed, so regressions here are really non important.

However, here is an example :

@startflow
10 "START"
s 20 "FINDHW()"
s 30 "GetTable()"
s 40 "i=0"
s 50 "is i==TopologySize?"
s 60 "return result"
e 70 "is dynamic data for this C?"
e 80 "Find Data on the heap for C"
n 90 "Can the buffer accommodate?"
@endflow

But once again, we don't care about this feature.

[matthew16550]

And I think these from FileFormatOption could move into ImageBuilder?

• hoverColor
• preserveAspectRatio
• svgLinkTarget

So far as I can see nothing is using these in FileFormatOption?

• affineTransform
• scale
• watermark

[arnaudroques]

And I think these from FileFormatOption could move into ImageBuilder?

• hoverColor
• preserveAspectRatio
• svgLinkTarget

Yes, I agree.

So far as I can see nothing is using these in FileFormatOption?

• affineTransform
• scale
• watermark

Well, ok for affineTransform and scale. This is probably dead code. I think withScale() is unused, you can remove it.

About watermark, it's a different story. The code is not public but we use it on http://alphadoc.plantuml.com/doc/asciidoc/en/sequence-diagram for example (to have watermarks on the images).

So we need the withWartermark() method somewhere.

[matthew16550]

FileFormatOption is in the net.sourceforge.plantuml package, do we have a process for changing the public API?

No need to change the watermark code, I'm just looking for the "easy" simplifications around ImageBuilder.

[arnaudroques]

FileFormatOption is in the net.sourceforge.plantuml package, do we have a process for changing the public API?

This is a point where PlantUML is really bad: the public API.

We have done a try with net.sourceforge.plantuml.api, but the result is not that good. And I must say that we don't know how to design a well written API.

So, if you have ideas and if you like the subject, you can go on and propose something brand new.

Ideally, this new api would be packaged in com.plantuml.api.v1 (that's because on the long term we want to migrate from net.sourceforge.plantuml... to com.plantuml... and it would be a first step toward this).

Tell us it's something you can be interested in !

[matthew16550]

Some time in future if I have time yes I'd be interested.

Just now I'm thinking about API changes in FileFormatOption, the first idea I had was just to remove withHoverColor(), withScale() etc but that would break backward compatibility if anyone is using those methods. Perhaps it would be better to keep them?

The area I'm wanting to simpify is where UmlDiagram.exportDiagramNow() sets values in fileFormatOption. Following the new structure it should be setting those values in ImageBuilder but I think we can do that and keep backwards compatibility for FileFormatOption as well.