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

Make the minimap view shown match minimap #8

Closed
eboracus opened this issue Jul 29, 2015 · 12 comments
Closed

Make the minimap view shown match minimap #8

eboracus opened this issue Jul 29, 2015 · 12 comments

Comments

@eboracus
Copy link

The minimap view I'm getting on expose seems to be a lot narrower than the view shown by the minimap itself for each file (which is much more representative of how the file content looks). Is something squashing the width of the minimap when shown on the tabs? It'd be really nice if the view could fill the tab as it does in the file.

Minimap view on tabs:
expose_minimap_image

Minimap view in file:
expose_minimap_actual

@mrodalgaard
Copy link
Owner

I see it is especially bad on big screens with vertical panel split, because the minimap canvas gets really narrow compared to the total width.

The main problem is that the minimap has dynamic width, where as the tabs width are static for consistency and to make room for the title. Plus scaling of the minimap canvas does not look very nice - it gets fuzzy. But I still do not know why it gets even smaller then the original minimap canvas.

I'm experimenting with scaling according to full window width to accommodate this, but still don't overflow on smaller windows/screens.

@abe33
Copy link
Contributor

abe33 commented Aug 13, 2015

Hi there, let me thank you for this nice plugin, I had the idea to use the minimap as a file preview in the back of my mind for some time now so I'm really happy to see it being implemented.

As for the issue at hand, I think the point here is that you should not reuse the canvas from the minimap view, but create your own view instead:

minimap = minimapModule.minimapForEditor(@item)
minimapElement = document.createElement('atom-text-editor-minimap')
minimapElement.setModel(minimap)

As you correctly identified, the minimap canvas size is dynamic, but it's because it needs to update itself when the size change (ie. when opening find-and-replace). And as the minimap width is dependent of the text editor size on screen you'll end up with canvas that may be too small or too large for the tab. I haven't checked your stylesheet yet, so may be you use an overflow to prevent resizes for large canvas, but you can't workaround small canvas.
By using your own view you'll be sure to have a canvas at the size of your tab.

However, as is, I'm not sure how well a minimap element behaves outside of a text element (you may have tried it, so I'm interested in your feedback on that). I quickly checked the sources and I think that, as long as you don't call the attach method and append it your self in your tab, you're fine.

Let me know if yout need some changes in the minimap, but anyway, your package gives me some ideas to evolve the minimap to make it less tied to its text editor (so that package like yours can make use of the preview feature more easily).

@mrodalgaard
Copy link
Owner

Thank you @abe33 and I can say the same thing to you about atom-minimap - very nice plugin.

I experimented with a couple of solution before I settled with the current implementation:

  1. Rerendering the whole text editor - this was never going to work.
  2. Creating my own view from minimap, almost the way you describe in your comment - I dropped this as I never got it to draw and it seemed like too much of a hack without any documentation for external packages.
  3. Grab the minimap canvas if it exists by using minimapView.querySelectorAll('atom-text-editor-minimap /deep/ canvas')[0].
  4. Try to draw the DOM objects into a canvas as described by Mozilla. I even kept the solution in code for further experiments.
  5. Using an external library like rasterizeHTML.js.

As I figured most users would probably already have minimap installed, I just chose the "steal the canvas" solution 3.

I would on the other hand happily go back to solution 2 since I believe it's more correct. Especially if the expose package could depend on atom-minimap as an npm dependency instead of using minimapModule = loadedPackages['minimap'].mainModule to extract the already loaded package. Or an Atom consumable service.

Currently I use overflow: hidden; on the tab body to solve the potential large canvases. The minimap canvas size depends on the editor width, but as I wrote above the expose tabs needs to be statically sized. Even in the best case I would have to make a tradeoff between filling the tab view and resolution - with very narrow panes (if you understand what I mean).

When I get the time I will continue with solution 2 outside a text editor. But I probably need some additional notes from you.

@abe33
Copy link
Contributor

abe33 commented Aug 15, 2015

I started working on a stand-alone version of the minimap (see here), if you want to give it a try. In the end, the goal is to have stand-alone minimap's size being completely controlled by their view, so that you can put a view anywhere, give it any size and still have the associated model behaves properly when it comes to scrolling and redraw.
However, I'm wondering whether to expose these stand-alone minimaps in observeMinimaps or not so that they could also receive decorations from the various plugins. But as I'm not sure that it won't break some plugins that may assume that every minimaps is displayed in a TextEditorElement I'll probably leave it like that for the moment. The other thing I need to figure is how to deal with the various controls provided in the minimap view when in stand-alone mode (like the quick settings and drag features).

I also tried to figure how to use the current version in stand-alone mode based on the hypothesis I made in my previous post. There's a lot of things that break (like most styles and the rendering of the minimap when scrolling in the text editor), but I still get a result that should be enough for expose while waiting for the new API to be released.

I made a quick gist that you can put in your init script to test that, it'll work with both the old and new API so you can take inspiration from that if you want to support both versions. It appends a minimap view in the top right corner of the screen with a fixed size defined through CSS, just click on it to destroy the view:

atom.packages.serviceHub.consume 'minimap', '1.0.0', (minimapAPI) ->
  atom.commands.add 'atom-workspace',
    'minimap:test-stand-alone': =>
      # With the new API we can just get a new stand-alone minimap
      if minimapAPI.standAloneMinimapForEditor?
        minimap = minimapAPI.standAloneMinimapForEditor(atom.workspace.getActiveTextEditor())
        minimapElement = atom.views.getView(minimap)
        minimapElement.attach(document.body)

      # With the old API we have to reuse the existing minimap model
      else
        minimap = minimapAPI.getActiveMinimap()
        minimapElement = document.createElement('atom-text-editor-minimap')
        minimapElement.setModel(minimap)
        document.body.appendChild(minimapElement)
        minimapElement.attached = true

      minimapElement.style.cssText = '''
        width: 300px;
        height: 300px;
        position: fixed;
        top: 0;
        right: 100px;
        z-index: 10;
      '''
      minimapElement.addEventListener 'click', f = ->
        minimapElement.removeEventListener 'click', f
        minimapElement.destroy()

Especially if the expose package could depend on atom-minimap as an npm dependency instead of using minimapModule = loadedPackages['minimap'].mainModule to extract the already loaded package. Or an Atom consumable service.

There's an Atom service you can consume (see my snippet above). Also, when the packages set feature will be available, you could probably make use of it to bundle minimap with expose.

@mrodalgaard
Copy link
Owner

@abe33; I played around with the standalone minimap consumable that you recently released, which is definitely a better solution for me.

I still have questions/tasks that needs to be looked at:

@abe33
Copy link
Contributor

abe33 commented Sep 24, 2015

  1. Thank you again for that, I just published the patch with your fix.
  2. It's probably because the text editor has not been tokenized yet. I'm not sure how to workaround that but I'll take a look.
  3. Changing the width and height should be fine, if you've tried the snippet I posted above you should see how it behaves. Using CSS transforms you'll stretch/shrink the canvas instead of increasing/reducing the minimap space.
  4. Yes, right, actually I just wrote the minimum code to enable the feature 😅, I was waiting for some feedback before going deeper in this. It should not cause much trouble (except it pollute the DOM with crap) but ideally they should not be present. I'll take a look at that too.

@mrodalgaard
Copy link
Owner

Thank you @abe33. I have just published a package update using the minimap standalone consumable and it is definately a better appoach.

I will add the things I have observed using the standalone minimap to the minimap repo, where it belongs, when I get the time. Especially the scaling was a bit cumbersome.

It does however not improve much on the original issue by @eboracus. I have scaled it up a bit, but if I set it to fill the div it looks terrible.

@abe33
Copy link
Contributor

abe33 commented Oct 5, 2015

I will add the things I have observed using the standalone minimap to the minimap repo, where it belongs, when I get the time. Especially the scaling was a bit cumbersome.

Sure, I'll be waiting your feedback. Thinking again about this issue, maybe I can make the rendering settings (line height, char width and height) overridable at the minimap level so that one can tweak them at will when using a stand-alone minimap. I think that in your case, the proper way to use this would be to test whether there's a preferred width defined so that you can adjust the render size to match your container.

I may face some difficulties to work on that until Atom 1.0.20 gets released as there were some API changes in it that broke the minimap (I was using some private API that were removed), and as I'm always using an Atom version built against master I'm also forced to use a patched version of the minimap that is not released yet (I'm waiting the official release to push the patch).

I'm also wondering if the scroll indicator should be displayed in that mode, I was somehow displeased seeing it all alone on the right of the preview:

minimap-element-spec_coffee_-_users_cedric_github_minimap-_atom

@mrodalgaard
Copy link
Owner

58d5f4e removes the minimap control.

A helper method for scaling would be much appreciated, but I do not see how custom line height, char width and char height would help in my case.

@abe33
Copy link
Contributor

abe33 commented Oct 6, 2015

A helper method for scaling would be much appreciated, but I do not see how custom line height, char width and char height would help in my case.

Scaling a canvas is almost never the best approach as it gives a blurry result.
There's two things here that need to be addressed:

  • The user can have set the soft wrap at preferred line length setting enabled (as in @eboracus and my case). This will affect the width of the minimap's canvas as it always try to minimize the size of the canvas based on the text editor state.
  • The user can have tweaked the render of the minimap (for instance I use 1px for interline, char width and char height). This will also affect the size of the minimap's canvas.

If you you want to have a preview that is consistent with what the user see in its minimap you shouldn't touch anything (no scaling, no rendering tweaks). But it will result in the preview not fitting the tab width (in my case a line will never takes more that 80px, which leave a lot of empty space in the preview tab).

If you don't care being consistent with the minimap render, and rather trying to fill as much of the available space then tweaking the minimap render can be useful. Let's say that the preview tab has a width of 160px, if I have a preferred line length of 80 chars then you can compute that to fill the available space you can use a char width of 2px, but it will give a weird render if the other setting aren't changed accordingly, in that case, to be more consistent you should end up with a minimap with a 2px interline, 2px char width and 4px char height.

Now it's just a suggestion as I don't really know what should be the best here, I just think that scaling a canvas should be the last resort.

@mrodalgaard
Copy link
Owner

4a9691c does not completely solve the issue, but I believe it makes it a lot better - sharper, better sized and less of a hack.

If you are still not convinced, I will reopen this issue again.

@eboracus
Copy link
Author

Appreciate all the effort that has gone into this. I agree, even if its not optimum, it certainly looks a lot better filling more of the tab now. Nice job! 👍 🍻

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

3 participants