Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] [ARCH] Image preview draft #4162

Open
core-ai-bot opened this issue Aug 29, 2021 · 10 comments
Open

[CLOSED] [ARCH] Image preview draft #4162

core-ai-bot opened this issue Aug 29, 2021 · 10 comments

Comments

@core-ai-bot
Copy link
Member

Issue by emedvedev
Wednesday Jul 17, 2013 at 13:08 GMT
Originally opened as adobe/brackets#4492


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.

screen shot 2013-07-17 at 8 07 57 pm


emedvedev included the following code: https://github.com/adobe/brackets/pull/4492/commits

@core-ai-bot
Copy link
Member Author

Comment by emedvedev
Wednesday Jul 17, 2013 at 13:17 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Wednesday Jul 17, 2013 at 17:30 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by emedvedev
Wednesday Jul 17, 2013 at 17:34 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Jul 19, 2013 at 00:01 GMT


@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)

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Jul 19, 2013 at 00:04 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Jul 22, 2013 at 18:36 GMT


@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).

@core-ai-bot
Copy link
Member Author

Comment by emedvedev
Monday Jul 22, 2013 at 18:38 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Thursday Aug 29, 2013 at 13:42 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by eugeneo
Friday Sep 06, 2013 at 21:29 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by emedvedev
Friday Sep 06, 2013 at 22:53 GMT


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

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

No branches or pull requests

1 participant