On Fri, 30 Aug 2024 20:20:22 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - improved vertical scrolling >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - cleanup >> - navigation >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - whitespace >> - update + review comments >> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea >> - whitespace >> - 8301121: RichTextArea Control (Incubator) > > I was looking into what "editable" does in `RichTextArea`, and I think we > need a new name for one of the methods and might also need a second > convenience method. There are at least three possible states that we need to > consider: > > A. The model supports editing; the control's editable property is true > > This means that the document can be modified via the UI; it can be modified > by calling control methods from the app (or by calling the editing methods on > StyledTextModel, but apps don't typically do that). > > B. The model supports editing, the control's editable property is false > > This means that the document cannot be modified via the UI; it _can_ be > modified by calling control methods from the app (or by calling the editing > methods on StyledTextModel, but apps don't typically do that). > > C. The model does not support editing; it is read-only as far as the control > and the StyledTextModel base class are concerned; the control's editable > property is not relevant > > This means that the document cannot be modified via the UI; it cannot be > modified by calling control methods from the app (nor by calling the editing > methods on StyledTextModel, but apps don't typically do that). > > There are a couple of thoughts I have around this. > > First, `RichTextArea::canEdit` is not sufficient to distinguish these three > cases; it will return false for both B and C. There are methods in RTA that > say "if canEdit is false, then this method does nothing". That's correct for > case C, but some of those methods -- namely the ones not tied to a UI action > (e.g., `appendText`, `insertText`, `replaceText`, `applyStyle`, `setStyle`, > `clear`) -- will intentionally do something in case B. Consider adding a > second convenience method that returns `model != null && > model.isUserEditable` or similar, and use that new method to qualify whether > the non-UI mutator methods of RTA do anything. > > Second, `StyledTextModel::userEditable` doesn't seem like the right name for > the method in the model to convey that it is effectively read-only as far as > the control is concerned. Perhaps "writable" would be a better name, since > from the point-of-view of the caller of the StyledTextModel, that's what it > means. Maybe there is an even better name, but having "user" in the name is > misleading (and editable by itself would be too confusing, since that's the > name we want to keep for the control, and it doesn't have the same meaning). > Or you could call it "readOnly", but then that would flip the sense of the > boolean. Thank you @kevinrushforth . I just wanted to emphasize that unlike any other feature that goes into mainline, this module is an incubating module, with the primary goal of getting the new APIn the hands of the app developers, **in order to receive their feedback**. This makes the acceptance criteria a bit less stringent, as the incubator may incubate over several releases (or be dropped altogether). I would really like to target jfx24, even if the code and API has imperfections. And, as always, your feedback and suggestions are greatly appreciated! ------------- PR Comment: https://git.openjdk.org/jfx/pull/1524#issuecomment-2326881979
