CSSInlineEditor class should be renamed and moved to separate file #768

Activity

1. peterflynn commented on Apr 28, 2012

   

   peterflynn

   on Apr 28, 2012

   Member

   More actions

   Another option is to push down all the generic stuff into the InlineTextEditor base class (and maybe rename it). It already encapsulates the notion of having multiple Editors, so it doesn't feel like a stretch to also include the list UI (driven by TextRanges) and editor-switching logic. As it stands, InlineTextEditor feels like an awkward link in the inheritance chain, since it provides some of the tools for a multi-Editor UI but not all of them.

   This would be basically equivalent to doing what Glenn said above, and then merging that new MultiFileInlineTextEditor with the existing InlineTextEditor (since it also implies multiple editors).
2. gruehle commented on Apr 30, 2012

   

   gruehle

   on Apr 30, 2012

   MemberAuthor

   More actions

   I agree that InlineTextEditor is awkward at the moment. However, there are some key questions that need to be answered about it:

   1. Will we support multiple text ranges visible at the same time? (this is in the Northstar, but hasn't been committed for Brackets yet)
   2. Do we want a base class that only shows a single text range (ie no list on the right)?
   3. Do we want a base class that has a file header, but doesn't have a text editor? (ie for showing other file types like image)

   I'm hesitant to merge CSSInlineEditor into InlineTextEditor without knowing the answers to these questions. Simply moving CSSInlineEditor to a separate file (and giving it a new name) has immediate benefit and low cost, and does not rule out further refactoring/merging with InlineTextEditor.
3. peterflynn commented on Apr 30, 2012

   

   peterflynn

   on Apr 30, 2012

   Member

   More actions

   Would it be reasonable to hoist the multi-editor-related pieces of InlineTextEditor up into that new class then? The main thing that bugs me about InlineTextEditor is that its name implies it hosts a single text Editor, but its API is designed around having multiple Editors. (Mainly I'm thinking of _syncGutterWidths(), sizeInlineWidgetToContents(), and the other code that references the editors array).

   Or if that feels like too much churn at once we could always drop a TODO in there and file a spinoff bug for that phase of the cleanup...
4. peterflynn commented on May 3, 2012

   

   peterflynn

   on May 3, 2012

   Member

   More actions

   The current refactoring is a definite improvement, so we can close this. I still think we should clean up / clarify / maybe remove InlineTextEditor at some point, but it can wait.
5. peterflynn

   closed this as completedon May 3, 2012