Major redesign of 'SnapshotView'

#407 Merged at 1107cff
Repository
nicolaiparlog
Branch
default
Repository
controlsfx
Branch
default
Author
  1. Nicolai Parlog
Reviewers
Description

To make the "select an arbitrary node" idea work properly I had to make some API breaking changes. Most importantly the 'selectionValidProperty' is missing and the selection's coordinates are interpreted differently. As the deployed version of the control was still unfinished (by me) and had a lot of bugs, I don't assume, there are many users, though.

The control is almost ready to be merged but depending on the answers to my questions below, some details might still need to be changed. I answered 2. (constructor removed), 4. and 5. (used @see ...property()) myself but 1. and 3. might still lead to API breaking changes.

Comments (13)

  1. Nicolai Parlog author

    Will do. Here are my questions, then:

    1. Pane

    From the demo I gathered the experience that many nodes behave "better" (e.g. if the control is resized) if they are embedded in a pane before given to the snapshot view. But I would not like to implement the skin such that it automatically does that because then 'displayedNode.getBoundsInParent' becomes somewhat useless as the parent is a pane of typically the exact same size.

    As it is now, the parent is the grid pane which spans the whole control, so those bounds are meaningful and can be used to compute stuff from the selection coordinates. Precisely this is already deeply baked into the code, as well, so I'm even less interested in changing it.

    Any suggestions?

    2. Remove Image URL Constructor (edit: I removed the constructor)

    Mainly because of the business with the pane described above I would like to take this one out. To work properly it would have to embedd the created image view in a pane and that would confuse the user. Better leave the decision up to her.

    3. Down With Null!

    I don't like null and try to avoid it whenever possible. Unfortunately at the moment I'm modeling an empty selection by using null. I would prefer 'Rectangle2D.EMPTY' and forbid null altogether (i.e. throwing an NPE if someone tries to write it to 'selecrionPoperty').

    Would this be ok or is it too surprising compared to other controls and their behavior (e.g. ImageView.setViewport')?

    4. CSS (edit: I figured it out)

    I'm stumped in how to implement CSS support for the control. E.g. the colors of the selected area, the dividing line and the outer area could be made editable this way. Could someone point me to a good resource on how to implement such a thing or point me to an example which does it following the best practice?

    5. Link to Property Comment

    I've seen in the JDK that comments of getters and setters also contain the description of the property holding the value. I think it should be like this (or at least the accessor should link to the property) and have previously done something similar by copy-pasting the property comment to the getter and setter. This violates DRY and bloats the source code. What would you recommend?

    1. Anirvan Sarkar

      @nicolaiparlog Overall the code looks good. I especially like HelloSnapshotView. About point 5, commit a7e3fa6 added -javafx javadoc option to ControlsFX build script which does this.

      1. Nicolai Parlog author

        Thanks for the thorough (and apparently still ongoing) review. I'll fix these things later tonight and update the pull request.

      2. Nicolai Parlog author

        About 5: If I understood your link correctly, I just have to remove the comments from the getters and setters, right?

        What do you think about the comments on the property fields? They currently link to the public ...Property() method. This keeps the property documentation in one place but also allows readers of the code to see it while inside some other part of the code (at least in Eclipse, where you can follow the link in the Javadoc view). Still bloats the code, though.

      1. Nicolai Parlog author

        This addresses 3, right? Yes, I considered it. (I'm actually a big fan of Optional.) But I decided against it because it would be very different from, e.g., ImageView.setViewport. I also think Rectangle2D.EMPTY would be a less intrusive and very intuitive way to represent no selection.

        I would've done this already but I don't see the full picture and can't compare this to the rest of the API. If this would stick out because it's the only control avoiding null, then this could be an argument against doing it. Hence the question. :)

  2. Nicolai Parlog author

    To avoid unnecessary work I'll clean up all the conflicts at once when its time to merge...

  3. Samir Hadzic

    @JonathanGiles Have you some time to review this? It seems clean to me. Maybe we can merge it and then open feature/improvements issue if necessary?

  4. Samir Hadzic

    I've looked a bit and the code and javadoc look very fine. Haven't fully tested it since I'm no expert in SnapshotView (haven't very much tested the old snapshotView) but I think we can merge it and then open issues/feature improvements.

    This merge will also resolve #452 .

    @JonathanGiles and @eryzhikov are you ok with that?

  5. Nicolai Parlog author

    Thank you @shadzic . I'm still following this and will gladly fix issues or implement enhancements. Would love to see this in the next release! :)

  6. Nicolai Parlog author

    Wohoo, Merged! Awesome. :)

    I do not plan to simply dump this control and leave - instead I would like to maintain it as well. I am not closely following the ControlsFX issues but I should get a notification if I'm mentioned anywhere. So if any issues are created which address this control and you would like me to look at it, just mention me and I'll do just that.