search

eclipse-sprotty / sprotty-server

Server implementation for the Sprotty diagramming framework
https://eclipse.org/sprotty
Eclipse Public License 2.0
23 stars 19 forks source link

Improvement: LayoutOptions aligment should be enums rather than String #33

Closed kdvolder closed 4 years ago

kdvolder commented 4 years ago

This concerns the Java api of class org.eclipse.sprotty.LayoutOptions.

There are some methods there to set options like

  public void setVAlign(final String vAlign)
  public void setHAlign(final String hAlign)

It is very hard to discover what 'magic' string values are valid parameters for these options. (We had to find/read the code of sprotty typescript for VBoxLayout and HBoxLayout to discover this).

It would be much nicer of you would use enum types here (as is done in other places of the api). Bonus points for adding meaningful documentation to the enum constants, but just having the values explicitly enumerated, would go a long way in helping a new user of the api understand what these methods expect.

kdvolder commented 4 years ago

Took a peek at the commits out of curiosity and noticed... This jdoc comment may be a copy-pasting error?

                /** 
         * Elements are aligned on top of each other left to right direction 
         */

I.e. 'left to right direction' was accidentially left behind?

JanKoehnlein commented 4 years ago

Fixed it. Thanks for the pointer