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

Feature: Screenshot of video #2221

Merged
merged 9 commits into from
May 30, 2022
Merged

Feature: Screenshot of video #2221

merged 9 commits into from
May 30, 2022

Conversation

hockerschwan
Copy link
Contributor

Pull Request Type

  • Feature Implementation

Related issue
Closes #1027

Description
Save as PNG / JPG, full resolution of the video (e.g., 1920x1080)
Keyboard shortcut (currently using "U")
Operating system's "Pictures" folder as default save location
File name customization / subfolder

Screenshots

Testing
Tested Local / Invidious and Dash / Legacy

Desktop

  • OS: Windows 10 21H1, Ubuntu 20.04

Additional context
File name pattern tooltip is not looking great ( can't use newline )

@PrestonN PrestonN enabled auto-merge (squash) April 25, 2022 10:16
auto-merge was automatically disabled April 25, 2022 10:52

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) April 25, 2022 10:52
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 25, 2022
auto-merge was automatically disabled May 2, 2022 15:54

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) May 2, 2022 15:54
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all this is very very cool!

  • Press U > Click on Take screenshot it works both in this sequence
  • Click on Take Screenshot > Press U takes screenshot when u click but doesnt when u press U
  • Could u rename Take Screenshot to Screenshot
  • I would like to see a toggle in the settings. Something like Enable Screenshot. After u enabled it all the settings related to screenshot will be visible and the icon in the player also will be visible after enabling the toggle.
  • Would like to see a toggle 'Ask for download path' just like in the download settings. This will prompt the user to manually chose the download path every time they take a screenshot.
  • Screenshots folder path should be editable for someone to type a path in but shouldnt be editable when Ask for download path is enabled

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 23, 2022
auto-merge was automatically disabled May 24, 2022 15:04

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) May 24, 2022 15:04
@efb4f5ff-1298-471a-8973-3d47447115dc

@hockerschwan ready for review again?

@hockerschwan
Copy link
Contributor Author

Yes please.
Added "Save As" dialog ( modified showSaveDialog in utils.js and src/main/index.js to show modal window. existing dialogs such as "Export History" etc. remain modeless )

Click on Take Screenshot > Press U takes screenshot when u click but doesnt when u press U

This happens with other buttons. Click pause or mute button and you can't use shortcut.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels May 24, 2022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

* Screenshots folder path should be editable for someone to type a path in but shouldnt be editable when Ask for download path is enabled

This was still missing but i think that something like this is a nice to have but not really necessary. I'd like to hear from the other if they agree or disagree on this

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start with simple variable naming issue
Will test feature locally later

Will review rest of the code when I really have time (busy~~)

src/main/index.js Outdated Show resolved Hide resolved
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 27, 2022
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Settings visibility tested
  • Save in subfolder tested (with %Y%M%D/%Y%M%D--%H%N%S)
  • Ask for folder tested
  • Keyboard shortcut (U) tested
  • On player button tested
  • image format tested
  • image quality tested (LOL at 0%)

What else should be tested?

0% jpg looks great in file size (only) 😆
20220527--120129

Update: also tested file pattern variables with %Y%M%D/%Y%M%D-%H%N%S-%T--%i--%s-%t

auto-merge was automatically disabled May 27, 2022 08:29

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) May 27, 2022 08:29
@hockerschwan
Copy link
Contributor Author

Isn't that beautiful?

Does "Pictures folder" (like "C:\Users\[user]\Pictures") work on mac when screenshotFolderPath is empty ?

@PikachuEXE
Copy link
Collaborator

How do I make it empty?

image

@efb4f5ff-1298-471a-8973-3d47447115dc

@PikachuEXE did u try to use Thunderbolt?

@hockerschwan
Copy link
Contributor Author

That's the default and working as intended, thanks

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I miss something untested this can be merged

@PikachuEXE
Copy link
Collaborator

@absidue @ChunkyProgrammer ?

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels May 30, 2022
@PrestonN PrestonN merged commit ddce28e into FreeTubeApp:development May 30, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 30, 2022
@hockerschwan hockerschwan deleted the screenshot branch June 13, 2022 04:42
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

Successfully merging this pull request may close these issues.

Ability to take screenshot of a video
5 participants