-
-
Notifications
You must be signed in to change notification settings - Fork 888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Future ImageParameter refactorings #501
Comments
Good point! +1
Sometimes a single text diagram generates several images.
For the record, the only use of this
Agreed. +1
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:
Point 2 and point 3 should be IHMO two separate PR. Does it sound good ? |
Agreed, many little PRs here. Small steps so it's as obvious as possible not changing functionality. |
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?
PSystemCute & PSystemTree are moot I guess as they are not enabled. |
Yes, sure!
This is stuff I should probably remove now. |
Would it make sense to merge |
Sure, yes. |
Do you have a thorough set of test cases for Flow diagrams? There are many subtle differences between My basic test case is identical: @startflow
foo "flow"
@endflow |
Actually, Flow is an old try that we should removed, so regressions here are really non important. However, here is an example :
But once again, we don't care about this feature. |
And I think these from
So far as I can see nothing is using these in
|
Yes, I agree.
Well, ok for About So we need the |
No need to change the watermark code, I'm just looking for the "easy" simplifications around |
This is a point where PlantUML is really bad: the public API. We have done a try with 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 Tell us it's something you can be interested in ! |
Some time in future if I have time yes I'd be interested. Just now I'm thinking about API changes in The area I'm wanting to simpify is where |
I'd like to talk over my next ideas for simplifying
ImageParameter
.ImageBuilder
then all the diagrams usingnew 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.Simplify or remove the
index
param. I haven't seen where the value is actually used?Use
diagram.seed()
inwriteStyledImage()
and remove theseed
param. We already pass in the value fromdiagram.seed()
in most places except forGitDiagram
,JsonDiagram
&NwDiagram
which use0
but I assume are safe to usediagram.seed()
?Other diagrams that do not use the above new methods are passing in a mixture of
diagram.seed()
,42
and0
. 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.Looking at the other big
ImageParameter
constructor, it's used byHelp
,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 inImageBuilder
something likewriteImageWithDefaultStyle(UDrawable drawable, OutputStream os, FileFormatOption fileFormatOption)
plus maybe a few params for values that are not constant.At this point the only place
ImageParameter
is used will be insideImageBuilder
so perhaps the fields ofImageParameter
could be moved toImageBuilder
andImageParameter
can be deleted?Then
writeImageTOBEMOVED()
should be simple to remove, its used from a few places inImageBuilder
and a few places outside that will need small adjustments first.Thoughts?
The text was updated successfully, but these errors were encountered: