[ARCH] Image preview draft

[emedvedev]

Hi!

It's not a final pull request, but I wanted to start a discussion about the feature. I've implemented the image preview in Brackets, but I don't like the way it's currently done. That's how it works:

• DocumentManager sends file path if it's an image instead of reading it as text;
• Document has a "template" parameter which is supposed to point to a custom rendering method;
• Editor substitutes everything inside the CodeMirror wrapper for a custom template if the template name is set in the Document.

Now, the emphasised part is something I strongly dislike, but I couldn't find a way around it. Opening an image as a document feels like a natural way of doing this (that's how it's done in Espresso.app, for example); opening a modal for image file types, while a viable alternative, feels inconsistent in terms of user experience and blocks the UI.

As for the current architecture, Brackets is strongly tied to CodeMirror and there is no way of consistently working with the document area without using at least the wrapper (which is understandable, Brackets being a text editor and all). It would be perfect to have not just an Editor class, but a Viewer class with custom templates (somewhat like I did it in this fork) as well, and an ability to switch between the two depending on a file type, but I'm not familiar with Brackets well enough to even know if it's needed and supported by the core team, so I did what I did.

Currently, there's a few things I certainly need to do with this request before making it suitable for merging:

• code conventions (really sorry for that one!)
• template events (or at least a "before render" event for the status bar)
• template files (right now there is a temporary and ugly workaround)
• status bar (something has to be done instead of the hack in the current version, but I'm not sure what)

But before doing all this I need to make sure somebody actually needs this, and maybe somebody from the core team could help out with Editor/Viewer (or at least discuss this in detail) and the status bar.

The ugly architecture aside, it works as intended on both platforms: shows an image preview and dimensions in the status bar. It could be a start, I guess.

Regards,
Ed.

[emedvedev]

Also, context menu on images works in OS X now, because there is no error modal anymore.

[JeffryBooher]

Hi @warabe,
Are these changes required for your extension (https://github.com/warabe/brackets.image-viewer)? I'm not sure why there is an extension and a pull request that do the same things.

I don't think we would take this until it works on all platforms but you're welcome to list the extension for OSX only.

[emedvedev]

Extension is something completely different: I wrote it first as a quick fix, it only works on OSX and substitutes the modal window (which is bad for UX, I believe). Just disregard it. :)

After writing the extension I wasn't satisfied with it, so I tried to implement it in the core; that's what the request is about. The changes I'm proposing work on all platforms, I tested it under OS X and Windows XP in a Virtual Box machine.

[peterflynn]

@warabe, here's an approach that I think would be cleaner:

• LanguageManager already owns a mapping from file extension to info about what the type of file means. Extend that to allow registering a custom viewer in place of the regular text-editor view. (This should be extensible, so the 'image preview' viewer and list of image file extensions shouldn't be hardcoded in the LanguageManager module).
• Make DocumentCommandHandlers.doOpen(), the central bottleneck for opening files, check with LanguageManager to see if a custom viewer is needed. If so, skip the whole getDocumentForPath() block and just ping EditorManager to invoke the viewer.
• EditorManager becomes a more generic manager of the "editor area," which might contain either a traditional Editor or an instance of a custom viewer.
• I think we could leave the status bar out of it -- when no Editor is shown it would disappear (same as today), leaving the custom viewer to occupy the entire area. Image dimensions could be shown somewhere within the viewer's own UI.
• We'd have to think more carefully about what DocumentManager.getCurrentDocument() should return when one of these is selected, and whether you should be able to add them to the working set. All the answers I can think of stand a chance of breaking somebody's assumptions...

What this doesn't give us is the ability to create custom editors that replace the default text editor, with full integration with Brackets' unsaved-changes tracking, undo, etc. In general, that runs contrary to the Brackets philosophy of code-first with visual widgets shown within the code view, but I could still imagine cases where that's what an extension would want. That seems a lot less urgent, though, and to do it cleanly probably requires a new implementation of Document to remove the dependency on CodeMirror (see CodeMirror document linking research). So I think it's fair to avoid tackling that area for now, and focus on read-only viewers.

(cc @njx, @gruehle in case you're interested in any of the architectural discussion here)

[peterflynn]

Actually, thinking further... one refinement to the above proposal:

In this new, alternate doOpen() codepath, generate a special "read-only" flavor of Document (e.g. a subclass). This would not be backed by a CodeMirror instance, could never have unsaved changes, and would assert / throw errors when any text-related APIs are called. But it would be a valid, fairly safe standin that we could return from getCurrentDocument() and other such places. That also gives us a cleaner path forward to a future where custom viewers might allow editing too.

[peterflynn]

@warabe closing this for now. Thanks for getting the discussion going! We talked this over in our architecture weekly today and feel like the approach outlined above is sound. Let us know if you're interested in working on that. (I've linked this to the image preview user story so the notes don't get lost).

[emedvedev]

@peterflynn I'm going to work on that for sure, most likely this week; I'm probably going to need some help—I'll open a new request as soon as I'm close to the outlined approach.

[lkcampbell]

Ignore the above reference, I accidentally typed the wrong issue number.

[eugeneo]

@warabe Are you working on this? I have some interest in this extension story (albeit not for images).

[emedvedev]

@eugeneo Not right now, but you can review the pull request.