Skip to content

Modulate model and view#242

Closed
JordanMartinez wants to merge 8 commits intoFXMisc:masterfrom
JordanMartinez:modulateModelAndView
Closed

Modulate model and view#242
JordanMartinez wants to merge 8 commits intoFXMisc:masterfrom
JordanMartinez:modulateModelAndView

Conversation

@JordanMartinez
Copy link
Copy Markdown
Contributor

A few questions I have

  • What should StyledTextAreaModel access-privilege be: public, protected, or package-private? Others could build on top of the model and the view.
  • I'm guessing the style-related methods for StyledTextAreaModel should be public (e.g. getInitialStyle(), getInitialParagraphStyle(), getApplyStyle(), getApplyParagraphStyle(), isPreserveStyle()). Is that correct?
  • Should the view be only concerned with view-related things? Or do you want to use it as a Proxy to StyledTextAreaModel? In other words, should I remove all the model-related methods from StyledTextArea so that one must access the model to change the model and the view to change the view?

…om its View (StyledTextArea). Note: TextEditingArea's `isEditable()`-related methods were removed since flag is only respected when user changes model via View, not via API calls.
…related component; updated Constructor javadoc
…g has now been changed so that exposing it is no longer necessary).
…ved unneeded import that wasn't removed in skin-removal commits.
…st segment's length should be 0. No need to waste time in `Paragraph#length()`.
@TomasMikula
Copy link
Copy Markdown
Member

The refactoring looks mostly good, except for some minor things (see below). I'm not sure though that the fix should be to change the Paragraph#equals method. Let's first focus on getting in the refactoring and discuss the fix then.

applyStyle and applyParagraphStyle are not needed in and don't belong to the model. Also, I think the model should not depend on anything JavaFX. Note that, however, applyStyle and applyParagraphStyle do depend on Text and TextFlow.

I also think the interaction with the clipboard should not be in the model (and as a consequence, codecs will not be necessary in the model either).

Why do constructors now take model instead of document? I thought each area will create its own model, given a document.

The Behavior does not have to take model as an argument, but it should get the model from the given area. You are giving the caller of new StyledTextAreaBehavior(...) a chance to screw up by giving area and model that are unrelated.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

The refactoring looks mostly good, except for some minor things (see below). I'm not sure though that the fix should be to change the Paragraph#equals method. Let's first focus on getting in the refactoring and discuss the fix then.

Gotcha. I'll work in the changes tomorrow.

Why do constructors now take model instead of document? I thought each area will create its own model, given a document.

Since it's been a few days since I did this, I don't recall perfectly why I did some things. However, I think that since the initialStyle and applyStyle-related items were placed into StyledTextAreaModel, I thought it'd be easier/more convenient for cloning to use the model itself to create a copy of itself (meaning, it passes the EditableStyledDocument to the newly-constructed model but allows the new model to create its own caret/selection values) and then just use that. In light of your comments about applyStyle, and Behavior, I see that's not what should be done.

Additionally, I think I started to see StyledTextAreaModel more like the model used for an area's content (i.e. EditableStyledDocument) and less like the model used for the specific view. So, that distinction might have also been why I refactored things the way I did.

I also think the interaction with the clipboard should not be in the model (and as a consequence, codecs will not be necessary in the model either).

Ok.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Ok. I've accounted for your comments in a new branch. See #243.

Closing this one.

@JordanMartinez JordanMartinez deleted the modulateModelAndView branch January 24, 2016 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants