Beta of SelectableImageView

#240 Merged at 22ca916
Repository
Deleted repository
Branch
selectable-image-view (cde491177224)
Repository
ControlsFX
Branch
default
Author
  1. Nicolai Parlog
Reviewers
Description

The SelectableImageView is more or less finished. What still needs to be done:

  • CSS-styling of the selection (I need some help on this)

  • I'd like to make SelectableImageView.selectionChangingProperty() read-only but see no way to do this as SelectableImageViewBehavior needs to set it but is in another package (which rules out default visibility)

  • it should be discussed whether SelectableImageViewBehavior.getTolerance computes the area around the selection's edge where resize cursors are shown in a sensible way or should be changed

  • there seems to be an issue when the control could grow: in some cases it doesn't...

  • more testing, code review, ...

I am unsure about how I'm supposed to be managing my branch. Should I have merged default into my branch selectable-image-view before this request?

Comments (23)

    1. Nicolai Parlog author

      Yes. :( But also a lot of comments. ;) A large chunk of code (everything in controlsfx/src/main/java/org/controlsfx/tools/rectangle) deals with resizing rectangles based on user input. As I want these changes to uphold a fixed selection ratio I saw no way around writing my own "how-to-create-rectangles-from-user-input"-code.

  1. Jonathan Giles

    The first thing that needs to be done is work out what is public API and what is private implementation detail. Public API should stay in the current package, whereas private implementation should go into the equivalent impl package

    1. Jonathan Giles

      I would say that everything in the tools package can be make private implementation for now also?

      1. Nicolai Parlog author

        Yes, possibly. That stuff could really easily be properly unit tested. Afterwards it could be released into the wild if you see any use in that. Either way is fine with me.

        1. Jonathan Giles

          For now, lets hide it, and we can bring it out in the future if it is deemed useful.

          1. Nicolai Parlog author

            Ok, I'll move it. Should I create a new package impl.org.controlsfx.tools or is there a better place?

    2. Nicolai Parlog author

      I guess this addresses my question about the SelectableImageView.selectionChangingProperty(). It is public API as it indicates when selection changes are due to user interaction. This is useful if something which should be bound to the selection (e.g. a preview of the selected area as an image) is too expensive to be computed the dozens of times per second the selection changes when the user manipulates it. But the way it is now, the developer using this class might actually edit that flag which makes no real sense.

      1. Jonathan Giles

        In general my approach when receiving a new pull request is to try and strip it back to the bare essentials, and then build up from there. Therefore, lets try to expose only the bare amount of API in this pull request, and then work to improve the implementation after that. Could you try to reduce the public API and then update the pull request?

        1. Nicolai Parlog author

          I guess by reducing the public API you mean taking out public methods from SelectableImageView. Ignoring accessor functions there are only the following properties:

          • ObjectProperty<Image> image: The currently shown image.

          • BooleanProperty preserveImageRatio: Analog to ImageView.preserveRatio.

          • ObjectProperty<Rectangle2D> selection: The currently selected area.

          • BooleanProperty selectionValid: Indicates whether the current selection is valid (i.e. such that it can be shown). Could be taken out but I think this can be a useful information.

          • BooleanProperty selectionActive: Indicates whether the current selection is active (i.e. supposed to be shown). Could be taken out but than programmatically interacting with the selection might get akward.

          • BooleanProperty selectionChanging: Indicates whether the selection is currently changing due to GUI interaction. Could be taken out completely (no need to make it private) but I think in certain circumstances this is an essential information. Namely, when an expensive operation is bound to selection changes.

          • BooleanProperty selectionRatioFixed: Indicates whether the selection's ratio is fixed.

          • DoubleProperty fixedSelectionRatio: The selection's fixed ratio.

          • BooleanProperty selectionActivityExplicitlyManaged: Indicates whether the control or the user wants to manage the selectionActive property. I'm not too happy with this myself so taking this out is fine with me. But it is then not possible to ever have programatic control over whether a selection is shown or not. Perhaps a new flag which simply turns off all selection-related behavior (effectively making the control a simpler Version of ImageView) would be clearer.

          If you wanna get a better picture of how these properties interact, you could read the class comment. It explains those relations in all details. What do you think should be taken out?

          Anyway, it's late in Germany, so I won't be doing anything more today. ;)

          1. Jonathan Giles

            At present, nothing should be taken out. My comment was relating to all the rectangle API that should be moved. I'll review the remaining code after that is done.

  2. Jonathan Giles

    I merged this in, and I have just pushed a massive refactoring (I hope you don't mind). It's incomplete, but shows some of the ideas I would like to explore. Let me know what you think (and whether you want to carry it on or leave it to me).

    1. Nicolai Parlog author

      I looked into SelectableImageView and your changes to that class are pretty straight-forward. I'll spend more time on this later today and am curious about your changes to the skin - I'll get back to you then.

      Speaking of time: I realized my comment regarding my time for pursuing "this" wasn't overly precise. I still wanna contribute (and already have another control and some property-related ideas in the pipeline) but I already had trouble with the skin for the ImageView (as you can see from the bug I left in there) and was certain that I wouldn't get the generalized idea to work within a decent amount of time. I'm more than happy to stand by, let you implement the general idea and then "clean up". ;)

      Another thought: Don't you think it would be better to create a small class hierarchy? Something like AbstractSelectable<Node> which implements the core functionality (essentially the current content of SelectableImageView) and provides most of its properties only to subclasses. A general version SelectableNode extends AbstractSelectable<Node> could simply make all protected properties public. Other inheriting classes, like SelectableImageView extends AbstractSelectable<ImageView>, could additionally provide some specialized properties and also decide which of the protected properties can be made public in the current context.

  3. Jonathan Giles

    I am typing this on my phone, so I'll be brief. I don't think we want to create a hierarchy of different implementations - I think the current approach should be sufficient.

  4. Nicolai Parlog author

    I finally got around to peruse the code.

    Changes

    Looks to me like the only key change is using a node instead of an image view - most other changes follow from there. Since the refactoring is not yet finished, the current version has some bugs. Most of them seem to be related to node sizes (e.g. select an area on the rotating node) which is in fact the main reason I shied away from implementing your idea myself. Layout and resizing of nodes is definitely one of my weak spots.

    Next?

    How do you want to continue? Did you look at the rest of the code as well?

    If you did and going from ImageView to Node is the only major thing you wanna change, all that's left is fixing the thing. Unfortunately, the mix of my limited time and the big unknown of how to fix the layout still means, I'd rather not do it. But, again, I'm happy to watch you do the critical stuff and fix all the small things afterwards.

    About the name

    I generally like names that get more specific from back to front, like ArrayList, hence SelectableImageView. So I started thinking about SelectingNode or SelectableNode but that's just stupid because (a) everything is a node and (b) this sounds like this would be a special kind of node which can be selected.

    But the idea of putting the node somewhere else in the name sounds good to me. And it is true that the control is a selector so having that in there makes sense, too. (English is my second language so take this with a grain of salt.) But when I hear "range" I think 1D, like a range of numbers. So I looked it up and found out I was a little off. Looks like range somewhat catches the meaning but in that sense it is pretty similar to selection. Having both in the name looks like overkill but just NodeSelector is too imprecise because nobody's going to select nodes with this.

    Look at that! I wrote more about names than about code... :) But I'll keep thinking about both.

    1. Jonathan Giles

      It would be great if you had the bandwidth to take a stab at the bugs, but if you don't then I will when time permits. Ideally we'd get this into shape before 8.0.6 ships, but that depends on bug fixing, and currently my priority is on the decoration and validation work.

      1. Nicolai Parlog author

        I would love to see this in 8.0.6! :) And I will try to scrape the time together. (When is the release planned for?)

        I will work on the generalized version if you would not ship the one just for image views. These are my major issues with the generalization:

        1. I don't quite see the use case for it. Do you think this will be used often?
        2. I'm worried that the generalization reduces the APIs usability for what I think is its major use case: selecting areas of images. I'm specifically thinking of the frame of reference for the selection's coordinates. I wrote the control such that it uses the image's coordiante which I think is a must if you use it for that. I don't see how to do that if I'm just handling a node.

        The latter was actually one of the reasons why I proposed a small class hierarchie. The generalized version could then make all behavior public as-is, while the specialized image view could alter it where necessary.

        So, again, if you deem the generalization important enough to not ship the specific version, I'll try to make it work. And not as grudgingly as this might sound. ;)

        1. Jonathan Giles

          There is no date set for 8.0.6 yet. It'll be another month or two at least.

          I think the bounds of an image should be the same as the bounds of a node, so in that case I don't see a specific problem with the generalised approach.

          My preference would be to work towards the generalised approach rather than a specific case. In my view the generalised approach is actually much more useful than the image-based approach. I see it being beneficial for things such as drawing programs (selecting a part of a canvas), allowing people to take snapshots of a specific part of their drawing. My feeling is that the generalised approach will be used significantly more than the image-only approach.

          1. Nicolai Parlog author

            Ok, I'll get cracking.

            I'm not sure, whether we mean the same thing when we're talking about the image's "coordinates" and its "bounds". But we can sort that out later. I will make a new pull request when I have questions.

  5. Nicolai Parlog author

    Hi Jonathan, I've got an idea and would like to get your opinion on that. You convinced me that the use of this control is much broader than what I initially thought. With that in mind I wonder whether it should be designed such that it allows selections other than rectangles. It could have kind of a SelectionModel<T extends Shape> which holds the current selection and also the behavior to change it. The current code could be refitted for a SelectionModel<Rectangle>and we can later implement others.

    Should I implement it this way? I would actually simply go ahead and do it but it would add at least one interface and one class to the public API and by now I know that I shouldn't do so lightly. ;)

    Going down this road implies that the selections (which were Rectangle2Dand are now shapes) are now mutable which also has to be dealt with. But we can discuss that later.

    1. Jonathan Giles

      I think using a rectangle is sufficient for 95% of use cases, and I don't think it is worth the extra complexity to support the 5% of extreme use cases. I think the main thing for now is just bug fixing and polishing.