mattermost / desktop

Mattermost Desktop application for Windows, Mac and Linux
Apache License 2.0
2.02k stars 827 forks source link

Add native quicklook #506

Closed luckydonald closed 7 years ago

luckydonald commented 7 years ago

I confirm (by marking "x" in the [ ] below):


Summary Implement native quicklook in macOS

Steps to reproduce

Expected behavior Quicklook opens

Observed behavior A iframe-ish thing opens. If the window is very small, this is not very helpfull. grafik

Possible fixes

Use electron's capability to use macOS native Quicklook to display files. https://electron.atom.io/docs/api/browser-window/#winpreviewfilepath-displayname-macos

grafik

jasonblais commented 7 years ago

Thanks @luckydonald!

Clear steps to reproduce the issue: Open a document

Do you mean looking at the contents of a file in the file upload dialog which appears after you've clicked the paperclip icon to upload a file?

luckydonald commented 7 years ago

@jasonblais I meant sent files. They are a preview image and a filename next to it. Clicking on the thumbnail currently opens a built-in viewer (1st picture). It should instead open it in quicklook.

image

jasonblais commented 7 years ago

Thanks @luckydonald, would you expect this to work on the browser as well? Or does the quicklook work if you try on Chrome for instance?

Sorry if it's a redundant question, I'm less familiar with the Mac platform

yuya-oc commented 7 years ago

The api is only applicable for local files. But the content area is almost same as Chrome, which behaves as a web browser. So I think it's impossible due to application architecture.

luckydonald commented 7 years ago

No, it is a native app/os component, which only electron exposes. So this would only be the desktop app.

yuya-oc commented 7 years ago

That's right, but I think there are no chances to call the API in webview (not BrowserWindow). At least, we need to steal events from webapp's ajax requests. If you have any way, feel free to reopen.

luckydonald commented 7 years ago

So, if I understand correctly, the app is just like an <iframe> for any normal mattermost website? What a bummer.

Maybe it is possible anyway. To say it first, I am no node.js guy, and barely know electron from the dev side.

But reading about ipc-message, communication seems possible, even simple.

Extending the example with my javascript haxx:

// In guest page.
const {ipcRenderer} = require('electron');
$('.preview_button').on('click', () => {
  var data = {"event": "open_preview", "file": "example.png"}
  ipcRenderer.sendToHost(JSON.stringify(data));
})
// In embedder page, were the webview is in.
const webview = document.querySelector('webview');
webview.addEventListener('ipc-message', (event) => {
  data = JSON.parse(event.channel);
  console.log(obj)
  // Prints {"event": "open_preview", "file": "example.png"}
  if (data.event == "open_preview") {
    if (macOS) {
      // quicklook code
    } else {
     // e.g. open it in a seperate window, allowing to scale the preview window independent.
    }
  }

})
webview.send('ping')

Not sure though.

I also found <webview>.executeJavaScript, if that code would be needed to be injected, i.e. because modifying the code which is the same in normal browsers (it seems?) is not an option.

Edit: Also I can't reopen, just post another issue.

jasonblais commented 7 years ago

Thanks! Re-opening for now to continue discussion

yuya-oc commented 7 years ago

Thanks for suggestion. But I worry about two things.

1.Use case

If the window is very small, this is not very helpfull.

Why not enlarge the window? And I have not experienced the case Quick Look is used for online files. Quick Look is a native component, but I feel there is a doubt whether it's appropriate.

2.Possibility of realizing

I'm not sure whether the example make sense. Sure, we need to use ipc-messsage. But there are some concerns.

jasonblais commented 7 years ago

@luckydonald any thoughts on latest reply from Yuya?

luckydonald commented 7 years ago

Why not enlarge the window?

Because I'd have to shrink it afterwards again.

And I have not experienced the case Quick Look is used for online files.

Indeed they would need to be placed in a temporary folder

Quick Look is a native component, but I feel there is a doubt whether it's appropriate.

I think it is a tradeoff between "easy" and "feeling native".

2.Possibility of realizing

I actually don't really know how the app internals works, so I can't really answer that...

yuya-oc commented 7 years ago

I think it is a tradeoff between "easy" and "feeling native".

Sure, it's an important thing. If this application was an API-based app, perhaps I might choose Quick Look. But currently I think it would be hard as I had mentioned before.

jasonblais commented 7 years ago

@yuya-oc would this be more feasible if we moved to Redux infrastructure (#484) or a truly native app with React (Windows; Mac)

yuya-oc commented 7 years ago

@jasonblais I think Redux is not related for this. Redux merely manages the state. So the core structure, the combination of tabs and webview would be kept.

If it's truly native, it would be easier. But we need to consider about Windows because Quick Look doesn't exist. Anyway we have to implement something like previewer.

jasonblais commented 7 years ago

@luckydonald thanks again for the feedback, really appreciate it.

For now, would you like to contribute this in the feature idea forum so it can be discussed, upvoted and considered for a ticket?

This is something we can consider in the long-term, particularly if we moved to pure React Native apps for the desktop. But with the existing framework, it's harder to implement.

You get 10 votes in the feature idea forum, and each one influences the future of the project.

jasonblais commented 7 years ago

Please let us know if you have any further feedback, about the Quick Look or the app in general.

All feedback is invaluable.