Toggle fullscreen when pressing f #2159

Closed
opened 2019-01-03 15:43:45 +01:00 by ngeiswei · 7 comments
ngeiswei commented 2019-01-03 15:43:45 +01:00 (Migrated from github.com)

The Issue

Pressing f while watching a video doesn't toggle fullscreen.

Steps to Reproduce

  1. Launch a video.
  2. Press f.
  3. Press f again, nothing happened.

Expected Behaviour

Fullscreen should be toggled.

Actual Behaviour

Nothing happens instead.

Suggested Solutions

Implement toggling fullscreen when pressing f.

System Configuration

  • LBRY Daemon version: 0.30.1
  • LBRY App version: 0.26.1
  • LBRY Installation ID: 9ZfMD7MUia6AAvrTDx1MzxGpsEBxUcK3mSxou2VgCFrLSYWwyNavkCTrPBpo5GoTbK
  • Operating system: Linux (Linux-4.18.0-13-lowlatency-x86_64-with-debian-buster-sid)

Anything Else

Screenshots

Internal Use

Acceptance Criteria

  1. If not in fullscreen should go into fullscreen when pressing f.
  2. If already in fullscreen, should go into window screen when pressing f.

Definition of Done

  • Tested against acceptance criteria
  • Tested against the assumptions of the user story
  • The project builds without errors
  • Unit tests are written and passing
  • Tests on devices/browsers listed in the issue have passed
  • QA performed & issues resolved
  • Refactoring completed
  • Any configuration or build changes documented
  • Documentation updated
  • Peer Code Review performed
<!-- Thanks for reporting an issue to LBRY and helping us improve! To make it possible for us to help you, please fill out below information carefully. Before reporting any issues, please make sure that you're using the latest version. - App releases: https://github.com/lbryio/lbry-desktop/releases - Standalone daemon: https://github.com/lbryio/lbry/releases We are also available on live chat at https://chat.lbry.io --> ## The Issue Pressing `f` while watching a video doesn't toggle fullscreen. ### Steps to Reproduce 1. Launch a video. 2. Press `f`. 3. Press `f` again, nothing happened. ### Expected Behaviour Fullscreen should be toggled. ### Actual Behaviour Nothing happens instead. ### Suggested Solutions Implement toggling fullscreen when pressing `f`. ## System Configuration <!-- For the app, this info is in the About section at the bottom of the Help page. You can include a screenshot instead of typing it out --> <!-- For the daemon, run: curl 'http://localhost:5279' --data '{"method":"version"}' and include the full output --> - LBRY Daemon version: 0.30.1 - LBRY App version: 0.26.1 - LBRY Installation ID: 9ZfMD7MUia6AAvrTDx1MzxGpsEBxUcK3mSxou2VgCFrLSYWwyNavkCTrPBpo5GoTbK - Operating system: Linux (Linux-4.18.0-13-lowlatency-x86_64-with-debian-buster-sid) ## Anything Else <!-- Include anything else that does not fit into the above sections --> ## Screenshots <!-- If a screenshot would help explain the bug, please include one or two here --> ## Internal Use ### Acceptance Criteria 1. If not in fullscreen should go into fullscreen when pressing `f`. 2. If already in fullscreen, should go into window screen when pressing `f`. ### Definition of Done - [ ] Tested against acceptance criteria - [ ] Tested against the assumptions of the user story - [ ] The project builds without errors - [ ] Unit tests are written and passing - [ ] Tests on devices/browsers listed in the issue have passed - [ ] QA performed & issues resolved - [ ] Refactoring completed - [ ] Any configuration or build changes documented - [ ] Documentation updated - [ ] Peer Code Review performed
tzarebczan commented 2019-01-03 15:46:27 +01:00 (Migrated from github.com)

Thanks for opening this issue! I agree, this would be a nice shortcut to have! Do you want to take a shot at implementing it?

Can we send you some appreciation for opening the issue?

Thanks for opening this issue! I agree, this would be a nice shortcut to have! Do you want to take a shot at implementing it? Can we send you [some appreciation](https://lbry.io/faq/appreciation) for opening the issue?
ngeiswei commented 2019-01-03 15:50:47 +01:00 (Migrated from github.com)

I could take a shot, though it might take me a while given how busy I am.

Question: would it be OK if it is hardwired?

Appreciation is always welcome :-)

I could take a shot, though it might take me a while given how busy I am. Question: would it be OK if it is hardwired? Appreciation is always welcome :-)
tzarebczan commented 2019-01-03 19:43:30 +01:00 (Migrated from github.com)

@ngeiswei hmm, not sure what you mean by hardwired. Maybe this previous PR to add a different shortcut would help: https://github.com/lbryio/lbry-desktop/pull/2003

@ngeiswei hmm, not sure what you mean by hardwired. Maybe this previous PR to add a different shortcut would help: https://github.com/lbryio/lbry-desktop/pull/2003
ngeiswei commented 2019-01-03 21:40:38 +01:00 (Migrated from github.com)

By hardwired I mean that the shortcut, here f, is specified in the code rather than in a config file that is accessible to the user.

BTW, I had a look at the code and I think I can handle it (especially if hardwired), just need to gather free time for it.

By hardwired I mean that the shortcut, here `f`, is specified in the code rather than in a config file that is accessible to the user. BTW, I had a look at the code and I think I can handle it (especially if hardwired), just need to gather free time for it.
neb-b commented 2019-01-03 22:04:47 +01:00 (Migrated from github.com)

Hey @ngeiswei

This is a great idea! This shouldn't be accessible to the user, but should just happen in the background like youtube does.

f should maximize the screen, but also minimize it.

Since we already have this functionality for double-click, it should be pretty straight forward to add.
You could add the event listener right below this line: https://github.com/lbryio/lbry-desktop/blob/master/src/renderer/component/fileViewer/internal/player.jsx#L110

We just need to make sure we remove it when the component is unmounted. And make sure we don't do anything if the input bar has focus (so we don't fullscreen when someone types an f into the search bar). You can check the focused value in the search reducer.

Hey @ngeiswei This is a great idea! This shouldn't be accessible to the user, but should just happen in the background like youtube does. `f` should maximize the screen, but also minimize it. Since we already have this functionality for `double-click`, it should be pretty straight forward to add. You could add the event listener right below this line: https://github.com/lbryio/lbry-desktop/blob/master/src/renderer/component/fileViewer/internal/player.jsx#L110 We just need to make sure we remove it when the component is unmounted. And make sure we don't do anything if the input bar has focus (so we don't fullscreen when someone types an `f` into the search bar). You can check the `focused` value in the `search` reducer.
ngeiswei commented 2019-01-03 22:13:02 +01:00 (Migrated from github.com)

Hi @seanyesmunt, yeah, player.jsx L110 is exactly where I had my eyes on.

We just need to make sure we remove it when the component is unmounted

I don't fully understand that but I guess it will become clear as I dwell deeper into the code.

Thanks for the other tips.

Hi @seanyesmunt, yeah, player.jsx L110 is exactly where I had my eyes on. > We just need to make sure we remove it when the component is unmounted I don't fully understand that but I guess it will become clear as I dwell deeper into the code. Thanks for the other tips.
neb-b commented 2019-01-03 22:15:36 +01:00 (Migrated from github.com)

@ngeiswei No problem.

I just meant that we need to remove the event listener when the react component leaves the page. We don't care if a user pressed f on the settings page. We do something similar here:
https://github.com/lbryio/lbry-desktop/blob/master/src/renderer/component/wunderbar/view.jsx#L39-L41

@ngeiswei No problem. I just meant that we need to remove the event listener when the react component leaves the page. We don't care if a user pressed `f` on the settings page. We do something similar here: https://github.com/lbryio/lbry-desktop/blob/master/src/renderer/component/wunderbar/view.jsx#L39-L41
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
LBRYCommunity/lbry-desktop#2159
No description provided.