Topics

DecoderPro panel cleanup

Svata Dedic
 

Hi,

I'd like to refactor the way how PaneProgPane is built up. No UI change intended in this phase, but rather code can be cleaned up.

Why doing this: in a later phase, I'd like to make _visual_ changes (see below). Any change, or bugfix will be more consistent, if done on 'canonical' code and easier to review.

The class actually contains 3 (almost) independent services:
- panel content builder
- the panel's logic itself (button binding, activation of functions)
- programmer process
I believe these three things are better separated, to follow prinicples of Go4. The current code is also quite messy (see newGridItem) and duplicated (newGroup, newColumn, newRow) as well.

Many arguments are passed to methods, which could be provided in context object, if a "builder" pattern is used. Initial prototype shows reduction of duplicated code AND GridBagConstraints manipulation code. Refactor is however not THAT successful since the LOC is +- the same before/after.

Why I am writing this mail at all: This is refactoring - should not affect end-user or API behaviour. Despite some testing, I need help from someone with more JMRI background to analyse the code for accidental changes; if found, the should be evaluated. Consistency fixes could be kept, but the goal is to just simplify the code, at this moment.

Anyone willing to collaborate / review ?

===

Second, I would like to separate XML reading code, from the *actual* layout building one. During analysis, it turned out that a decomposition will actually save some branches in the code.

Why doing this: In a later phase, I'd like to provide enhanced GridBag-based UI (some - IMHO - better alignment + anchors). I believe that UI should not be arranged as scattered tea leaves on the line.
And try to implement GroupLayout-based UI (except grids). GroupPanel COULD give better layout, better respect for L&F control spacing reducing the "scatter" effect even further.

The separation of xml / layout allows to provide "enhanced" impls as opt-ins, or small changes on a branch so users (who want) can play with it and later decide the fate of these changes for the official release. And makes way easier to review the changes, which should be mostly in the layout code itself.

Again someone more knowledgeable could point me on counter-examples in decoder configs early, so I could either accomodate the code or propose consistent layout change - when the result is presented to users, it could look "differently", but should be "more consistent" within the changed style.

===

BTW there's a non-JMRI motivation as well: I plan to plug into the refactored code in my own project, providing yet different UI. Cleaning JMRI code first could make my life quite easier.

Thanks,
-Svata

Bob Jacobsen
 

Happy to help with this! That code was created a long time ago, in part by agglomerating three separate programs, one of which was written in C++-transmogrified-to-Java, so it really is a mess.

Most decoder definitions factor content and presentation pretty well. Those ones use the presentation information from the xml/programmers directory and don’t have (or xInclude) <pane> elements.

https://www.jmri.org/help/en/html/apps/DecoderPro/CreateDecoderAdvanced.shtml

The presentation model of rows and columns was meant to allow people to pack pages simply. It doesn’t give really detailed layout tools, and the hope was that people would just accept simple, common layouts. But some of decoder definitions go beyond that to use tricks in their custom panes to i.e. align things between columns. Not sure what to do about that, it’ll be interesting to see what develops.

Separating the XML is also a good idea. One of the causes (but not the only one!) of the large memory footprint of DecoderPro with some large files is retained JDOM2 information. It’s been hard to track that down, as JDOM2 has a complicated retention model internally. Having well separated XML-read and functional code should help with that.

JMRI’s decoder-file (and roster file) reading tools were written before our ConfigXML idiom was developed. That might or might not be helpful for this; the decoder files really do provide a different metaphor with no need for the file to carry persistent class names, etc.

https://www.jmri.org/help/en/html/doc/Technical/XmlPersistance.shtml

Bob



On Oct 8, 2019, at 11:19 PM, Svata Dedic <svatopluk.dedic@...> wrote:

Hi,

I'd like to refactor the way how PaneProgPane is built up. No UI change intended in this phase, but rather code can be cleaned up.

Why doing this: in a later phase, I'd like to make _visual_ changes (see below). Any change, or bugfix will be more consistent, if done on 'canonical' code and easier to review.

The class actually contains 3 (almost) independent services:
- panel content builder
- the panel's logic itself (button binding, activation of functions)
- programmer process
I believe these three things are better separated, to follow prinicples of Go4. The current code is also quite messy (see newGridItem) and duplicated (newGroup, newColumn, newRow) as well.

Many arguments are passed to methods, which could be provided in context object, if a "builder" pattern is used. Initial prototype shows reduction of duplicated code AND GridBagConstraints manipulation code. Refactor is however not THAT successful since the LOC is +- the same before/after.

Why I am writing this mail at all: This is refactoring - should not affect end-user or API behaviour. Despite some testing, I need help from someone with more JMRI background to analyse the code for accidental changes; if found, the should be evaluated. Consistency fixes could be kept, but the goal is to just simplify the code, at this moment.

Anyone willing to collaborate / review ?

===

Second, I would like to separate XML reading code, from the *actual* layout building one. During analysis, it turned out that a decomposition will actually save some branches in the code.

Why doing this: In a later phase, I'd like to provide enhanced GridBag-based UI (some - IMHO - better alignment + anchors). I believe that UI should not be arranged as scattered tea leaves on the line.
And try to implement GroupLayout-based UI (except grids). GroupPanel COULD give better layout, better respect for L&F control spacing reducing the "scatter" effect even further.

The separation of xml / layout allows to provide "enhanced" impls as opt-ins, or small changes on a branch so users (who want) can play with it and later decide the fate of these changes for the official release. And makes way easier to review the changes, which should be mostly in the layout code itself.

Again someone more knowledgeable could point me on counter-examples in decoder configs early, so I could either accomodate the code or propose consistent layout change - when the result is presented to users, it could look "differently", but should be "more consistent" within the changed style.

===

BTW there's a non-JMRI motivation as well: I plan to plug into the refactored code in my own project, providing yet different UI. Cleaning JMRI code first could make my life quite easier.

Thanks,
-Svata


--
Bob Jacobsen
@BobJacobsen

Svata Dedic
 

Dne 09. 10. 19 v 18:26 Bob Jacobsen napsal(a):
Happy to help with this! That code was created a long time ago, in part by agglomerating three separate programs, one of which was written in C++-transmogrified-to-Java, so it really is a mess.
Perfect. The refactored code is https://github.com/svatoun/JMRI/tree/feature/decoderpro_layout_split

I am not sure what is the best path to proceed; on one hand, I'd like to propagate the refactored code to master to clean up things.

On other hand, I am almost sure that when adding GroupLayout alternative, the internal API changes a bit - as always as when plugging a completely different style. So from this perspective it's better to "consolidate" the code in a branch for a while.

With the layout changes, I'd like to address:
- bad alignment. DecoderPro dialogs are complex - so labels ought be right-aligned to allow quick top-down scan for the desired entry
- bad width: controls in the same 'group' (that's usually a <column>) should have the same width to avoid "scattered" feeling.
- bad spacing: most L&F mandate at least 6 or 8 pixels between adjacent input lines (controls)

As I looked into XMLs, there are a lot of _common_ stuff, but redefined in many decoders with slightly different wording; should be unified.

... and there are even more weird ideas ;)

-S.

Svata Dedic
 

Dne 09. 10. 19 v 18:26 Bob Jacobsen napsal(a):
Separating the XML is also a good idea. One of the causes (but not the only one!) of the large memory footprint of DecoderPro with some large files is retained JDOM2 information. It’s been hard to track that down, as JDOM2 has a complicated retention model internally. Having well separated XML-read and functional code should help with that.
Then the 'separation' should be extended to RosterEntry and PaneProgFrame as well: A quick scan in a heapdump shown that there are not many Content JDOM references referenced from individual PaneProgPanels (this is what I've refactored), but that the Frame keeps them for some reason. RosterEntry as well.
If those do not load content lazily / partially on-demand, they shouldn't keep Element refs after initial phase.

-S.

Svata Dedic
 

Dne 09. 10. 19 v 18:26 Bob Jacobsen napsal(a):

https://www.jmri.org/help/en/html/apps/DecoderPro/CreateDecoderAdvanced.shtml
I looked on how ZIMO uses the layout and I found a funny thing:
Layout comes from programmer definition, ZIMO decoder gives variable title working (i.e. various light stuff).
When looking on the outcome, I am tempted to wrap ZIMO's items into a titled pane (common label in the title, save words in control labels). BUT
- programmer definition can't assume similar wording of the placeholder items, since they can be, in fact, unrelated, or can be just a few of them to form a visual group,
- decoder can't influence the layout in this case, but "knows" it would be good to group them

Strange thing ! Someone has an idea how the decoder could provide "hint" for the layout templates ?

The presentation model of rows and columns was meant to allow people to pack pages simply. It doesn’t give really detailed layout tools, and the hope was that people would just accept simple, common layouts. But some of decoder definitions go beyond that to use tricks in their custom panes to i.e. align things between columns. Not sure what to do about that, it’ll be interesting to see what develops.
I'm not sure that either. The tricks have to fail (IMHO) as they rely on presentation of a variable to be the same size as a text (label), which can't be guaranteed.
If some validation feedback elements step in (i.e. possible error will be displayed on next line), this assumption will break at all.

With GroupLayout, it is *possible* to skip intermediate panels (that are there just for grouping), which allows to glue components on a row, across columns. Maybe an extension hint element/attribute would be helpful to express the intent clearly (e.g. alignY='item-name-of-the-leader-component'; disallowing forward references).

I am not sure if this is doable with across JPanel levels with GridBag even if I somehow hacked the algorithm in a subclass.

-S.