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

Dialog to confirm requested permissions #609

Merged
merged 6 commits into from
Nov 7, 2017

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Oct 4, 2017

Before submitting, please confirm you've

Please provide the following information:

Summary
Use session.setPermissionRequestHandler() to confirm permissions when they're requested.

Results of the dialog are saved to AppData\Roaming\Mattermost\permissions.json.
This implementation is naive. The dialog pause the application until selecting a choice.

Issue link
https://mattermost.atlassian.net/browse/PLT-7458

Test Cases

  1. Receive any messages in a channel.
  2. A dialog should appear to confirm Allow/Deny/Skip the 'notifications' permission.
  3. A desktop notification should appear after clicking 'Allow'.

Additional Notes
https://circleci.com/gh/yuya-oc/desktop/411#artifacts

@yuya-oc yuya-oc added this to the v3.8.0 milestone Oct 4, 2017
@jasonblais
Copy link
Contributor

jasonblais commented Oct 4, 2017

Thanks @yuya-oc!

I can ask one of our designers to help with a design mock up for it.
image

Questions:

  1. What options do we have for the popup? E.g. we could have
  • popup in the middle
  • popup in top left corner similar to browsers
  • popup, perhaps similar to desktop notifications in the bottom right corner
  • a bar at the top similar to the "blue bar" message sometimes displayed in Mattermost

image
2) The dialog pause the application until selecting a choice. Are there any improvements we can do?
3) Are there any other instances where this dialog would be needed (WebRTC video calls?)

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 5, 2017

  1. Options
  • popup in the middle
  • popup in top left corner similar to browsers
    Possible. But need to create a dialog with BrowserWindow instead of dialog.
  • popup, perhaps similar to desktop notifications in the bottom right corner
    Currently impossible. We can't choose allow/deny in desktop notifications without native modules.
  • a bar at the top similar to the "blue bar" message sometimes displayed in Mattermost
    Possible. But a little complex internal flow would be needed.
  1. Improvement for The dialog pause the application until selecting a choice
    The application stops because dialog uses synchronous flow. So we can improve that by using asynchronous flow. But it would be a little complex than synchronous one.

  2. Permissions:
    Sure. There are ‘media’(WebRTC), ‘geolocation’, ‘notifications’, ‘midiSysex’, ‘pointerLock’, ‘fullscreen’, ‘openExternal’.
    https://electron.atom.io/docs/api/session/#sessetpermissionrequesthandlerhandler

@jasonblais
Copy link
Contributor

Hey @yuya-oc, sorry for the delay on this.

Sent you a note on pre-release and added notes on this Google presentation: https://docs.google.com/presentation/d/1uyIQxcscGzKcXPHUMMb3Rg7Loxld1ztmOE7t4d0z_nA/edit#slide=id.g28d3453657_0_16

I think we're close after a minor change to the dialog design and positioning.

Regarding 2), not sure how much work this would be? Thinking of a use case where the user receives the dialog while the app is minimized, without realizing it until much later. It could be a bad user experience if they didn't receive any notifications during this time period.

@jasonblais
Copy link
Contributor

jasonblais commented Oct 30, 2017

1 - I'm still evaluating the options. Will get back to you
2 - @yuya-oc How much work would it be to switch this to asynchronous flow?
3 - 👍

For the dialog, are we able to get close to the Chrome "request permission" dialog?

image

This would mean

  • title: Mattermost App wants to:
  • text: Show notifications / Use your camera and microphone
  • icon: Bell for notifications / video camera for video calls (I can provide these as we use similar icons elsewhere in Mattermost)
  • buttons: Allow / Block

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Oct 31, 2017

2 - I need to consider the case where many requests are coming. At least a day would be needed.

Dialog: It would be more complex flow because it's combination of main and renderer process though, possibe by using Popover of react-bootstrap. https://react-bootstrap.github.io/components.html#popovers

@jasonblais
Copy link
Contributor

@yuya-oc How much more complex would it be?

The reason why having a "nicer" dialog would be good is that the majority of users will see this dialog (given most have desktop notifications enabled). So it's worth spending some the time to improve them. However, if it's a lot of work we can consider keeping the original design and just changing the texts.

The other dialogs we're adding (navigating to external pages, uncaught exception errors) are more rare and only a few people will ever see them.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 1, 2017

I feel it's difficult to describe enough in sentences... It's surely complex because two asynchronous things work together, so I need to manage the state in each processes. But the flow is almost similar in both cases.

However, I need to implement the dialog itself in addition to logical flow.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 1, 2017

Of course I agree to create nicer dialog.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 3, 2017

This is a prototype for the dialog. Icons are chosen from Bootstrap glyphicons for now. https://getbootstrap.com/docs/3.3/components/#glyphicons

dialog

@yuya-oc yuya-oc changed the title WIP: Permission request handler Dialog to confirm requested permissions Nov 4, 2017
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 4, 2017

Implemented the dialog with asynchronous flow. https://circleci.com/gh/yuya-oc/desktop/471#artifacts

@jasonblais
Copy link
Contributor

This looks great to me.

Just two minor questions/notes:

  1. I think the text isn't quite centered with the icon. If we're not limited by Electron, maybe move it by 1px up? No worries if not possible.

image

  1. I didn't grab a screenshot, but when you click "Allow" or "Block" there's a 1-2 second lag when the action is accepted. Not sure if we can reduce that time?

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 6, 2017

  1. It looks difficult for me. Possibly we can move it by 1px with hardcoding though, we also need to consider about other icons.

  2. I noticed that point. I think it's related to React's state management though, I could not find the actual problem.

@jasonblais
Copy link
Contributor

  1. Sounds good
  2. Okay, let's merge and see how it feels. We'll have a month to try improve it if needed.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 7, 2017

Rebased to solve conflicts. #600 is still working fine.

@yuya-oc yuya-oc merged commit dfe3d47 into mattermost:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants