Skip to content

Skin removal#213

Merged
TomasMikula merged 10 commits intoFXMisc:masterfrom
JordanMartinez:skinRemoval-Attempt3
Nov 23, 2015
Merged

Skin removal#213
TomasMikula merged 10 commits intoFXMisc:masterfrom
JordanMartinez:skinRemoval-Attempt3

Conversation

@JordanMartinez
Copy link
Copy Markdown
Contributor

This one's cleaner.

@TomasMikula
Copy link
Copy Markdown
Member

Is there something lost from StyledTextAreaView or StyledTextAreaVisual, when you keep them around?

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Is there something lost from StyledTextAreaView or StyledTextAreaVisual, when you keep them around?

I don't think so. I just didn't see the need to have them anymore, so I deleted them. Should I leave them there and just deprecate them?

@TomasMikula
Copy link
Copy Markdown
Member

No, sorry, I was looking at the commit where you inlined StyledTextAreaView and was wondering why you kept the original file around. Didn't realize you deleted it in the next commit.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Oh ok...
Looking back at them now, if I don't delete them, I get issues in StyledTextAreaView when trying to access CharacterHit because it's no longer in the skin package and I'm guessing you don't want it public.

@TomasMikula
Copy link
Copy Markdown
Member

Seems like the dispose() method is no longer needed. The Skin had to be disposed to stop observing the Control. Now the "control" is only observing itself (its own properties and its own EditableStyledDocument), so disposal is not necessary.

(In the future I would like to be able to provide an external document. At that point, dispose will need to be reintroduced to break ties with the document, if the document outlives the area.)

If you don't see any catch, we can remove the method. That also means we don't need to keep the subscriptions that are unsubscribed in dispose().

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Seems like the dispose() method is no longer needed.

Yeah, I was wondering about that...

Looking at its dependencies, would I remove all of the following?

  • Subscription subscription = () -> {};
  • manageSubscription()
  • manageBinding()
  • subscribeTo()

and then change all subscribeTo() to stream.subscribe( /* code */); and delete all manageBinding() references.

(In the future I would like to be able to provide an external document. At that point, dispose will need to be reintroduced to break ties with the document, if the document outlives the area.)

Yeah, the idea proposed in #152.

@TomasMikula
Copy link
Copy Markdown
Member

One more thing. Seems like StyledTextAreaBehavior doesn't need a direct reference to VirtualFlow. In the two places it is used, area can be used just as well.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

I was thinking it would be more precise if VirtualFlow was used. But then again, if it's the only child, I guess it doesn't really matter....

@TomasMikula
Copy link
Copy Markdown
Member

This was a lot of work, thanks!

TomasMikula added a commit that referenced this pull request Nov 23, 2015
@TomasMikula TomasMikula merged commit 45145ff into FXMisc:master Nov 23, 2015
@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Woohoo!

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.

3 participants