-
Notifications
You must be signed in to change notification settings - Fork 549
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
Check in "Save Current Frame" feature #1542
Conversation
Neat, that sounds like a handy feature to have. Especially if this is going to bring up a dialog window anyway, though, what do you think of having it pop up a file-save dialog and allowing the user to (a) name and (b) place the file however they choose? (It could start at the default output directory, and be pre-filled with the default filename, but this way the user isn't forced into either of those.) Then the results popup could be dispensed with, as the file-chooser dialog effectively serves as their notification. I'm not too wild about just dumping the file into a set location, without giving the user control over that location. Nor of forcibly overwriting previous output files without warning. The user may very well want to do something like, say, save the same frame# multiple times as they make changes to their project, then compare those different versions. (I also just hate message boxes. In general. Especially "everything's OK!" normal-status message boxes. The way I see it, if a piece of software is going to interrupt my work with something I have to acknowledge before I can continue, it had better be damn important. "I successfully did the thing you just told me to do by clicking an icon / pressing a keyboard shortcut!" is never going to qualify. It's not like this should be news, or come as a surprise to me. I'm the one who requested it, I want to be able to assume that it was done! There's no message box for "I just zoomed in on the timeline!" when you click "Zoom in". The only reason an application should bother me with the results of an operation I triggered is when something goes wrong and it isn't completed successfully.) |
I wonder if we could come up with a more subtle-er status indicator. Perhaps make the icon change to show a check mark or a cross mark for a few seconds to indicate whether the save has failed or not? Also, is it possible to have the 'grab frames to this directory' part in the preferences? And also have an option to save into a folder called 'screengrabs' or something in the current directory where the .osp file resides? Sorry if I'm asking for a little too much... :D |
The traditional location for such reports would be the status bar. You throw a message like "Saved frame $number image to file $path" down there, and the user can read it or not, but they're not forced (or able) to interact with it in any way. Status bar messages (and status bars in general) have been falling out of favor, but the current OpenShot main window layout does include one. (At least, it does on Linux. So I'm guessing it does on all platforms, since Qt is pretty consistent about that stuff.) To my knowledge, at the moment it's completely unused.
It's possible to have anything in the preferences, but:
|
@ferdnyc :
I hadn't considered this, but shouldn't be too difficult to implement consistently as this is implemented elsewhere in OpenShot.
I was attempting to make the frame grab as simple as possible, so dumping to the project directory or the user's home dir (which is also the defaults used elsewhere in OpenShot) seemed reasonable, at the time, but the overwriting of previous files should be handled better. The message box was so that the location of the frame grab was announced to the user (upon success) or that there was an error (upon failure). I considered those "damn important" 😆 I don't see a need for a dialog or notification of a change in zoom because the change is immediately apparent to the user as the visible timeline duration changes (self-evident). @peanutbutterandcrackers and @ferdnyc
The directory where the .osp files resides is the default for saved frames right now, which fails back to the user's home dir if the project hasn't been saved, just like the behavior that OpenShot exhibits for .osp files and exported videos.
I don't see a status bar on any of the instances of OpenShot that I've ever tried (starting at 2.3.x and up to the current dev versions). I only use AppImage/dmg releases on Linux and Mac OS X, so perhaps status bars aren't available in those. 🤷♂️ I do think that the save file dialog is a good idea, with more reasonable default filename (something like Again, not sure how soon I can implement these changes, but hopefully very soon! I'm not expecting these to come out with 2.4.2, so it's not like there's a rush. 🏃 💨 |
Huh, really? No, sure you do! There's one in your screenshot in #876 — as well as in @peanutbutterandcrackers' screenshot from that same bug. Like I said, it's not used for anything in OpenShot (currently), but the reason the main window has such a thick bottom border is that the bottom border is actually a status line. It's been sitting down there ignored and unused #ThisWholeTime, given no purpose except to hold the window-resize handle. (Plus, I cheated and loaded the |
Oh man! Color me dense! I always thought that was dead space that was part of the timeline! |
src/windows/main_window.py
Outdated
@@ -889,8 +896,21 @@ def actionSaveFrame_trigger(self, event): | |||
recommended_path = info.HOME_PATH | |||
|
|||
#log.info("Saving frame to %s/%s.png" % (recommended_path, self.preview_thread.current_frame)) | |||
framePath = "%s/%s.png" % (recommended_path, self.preview_thread.current_frame) | |||
log.info("Saving frame to %s" % framePath ) | |||
framePath = _("%s/Frame%04d.png" % (recommended_path, self.preview_thread.current_frame)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should use, and update, the project export_path
, instead of always starting at recommended_path
? Or perhaps even add yet another project file variable, to track snapshot_path
or whatever?
(See #1372 which created import_path
to track last-open-location for "Import Files...". But export_path
already exists, so you could just use that. Assuming users won't be annoyed that their exports and their frame snapshots affect the other's starting location.)
If nothing else, subsequent invocations of the dialog within the same session should endeavor to start in the same directory where the previous file was saved. Making the user navigate there from recommended_path
every single time can seem cruel, especially if they have to traverse a long path to the intended destination. Doing that once is tedious. Having to do it more than once feels like abuse.
Whether that persists beyond the current session... up to you, I don't really have an opinion on that.
@DylanC - I think I've addressed the concerns/suggestions EXCEPT the default path. I'll try to get the path issue sorted today. |
@N3WWN - There is no pressure. I just wanted to know where it was, as I couldn't follow the conversation very clearly. 😆 |
@DylanC - Don't tell me that! I'll put it on the back burner and never get to it! j/k 🤣 Okay, just checked in the commit which uses/updates the export_path consistently instead of defaulting to the project path (or HOME_PATH) each time. This was actually easier than I thought it would be. 🤷♂️ If you're good with this PR, and @ferdnyc and @peanutbutterandcrackers wanna sign off on it now that all items are addressed, I think we're good to go! |
Currently, even if the project is saved, it keeps on opening a pop-up and offers to save in the home-dir. I think I like the idea that this pop-up also keep track of the last directory used. However, if possible, I'd like the feature that I suggested in the comment above - to specify where the screen-grab should be kept. At least, for a saved project, a 'screen-captures' (or sth similar) directory in the same directory as the I know I might be asking for a little too much. Sorry. |
Okay, it took a little figuring out, but I was able to get the Save Frame icon right-justified while maintaining centered playback controls:
As you can see, I've used a dark icon instead of a light icon. Not sure if this is something that could (should?) be changed with the theme? Dark theme = dark icon and light theme = light icon? Regardless, I think this could be a future enhancement. I'd like to get the functionality into OpenShot first and we can tweak the appearance in different themes later.
I'm not sure how much extra juice we'd get for the proposed icon enhancements squeeze. 😁 Frame x and y: The current frame number behavior is currently how you describe the "Frame y" capture working. It is not possible to capture the frame from the clip (Frame x) with the current implementation of Save Frame since the frame in OpenShot is the (pre)rendered output frame. To capture just the previewed frame of an existing clip, a new timeline would need to be created, the clip added to the new timeline, possibly along with any cuts so that the current playhead location in the clip could be determined within the new timeline, and then the frame saved from there. We then have to ask if any other clip-specific changes should be applied on the new timeline (fades, effects, etc). If someone wanted to export a specific frame from a clip (not the timeline), I'd recommend they open a new project, import the clip that they want the frame from, locate the playhead accordingly and save the frame from there. Another way of accomplishing the same thing would be to add the ability to disable tracks. You'd then have to disable all track except the one with the clip you want a frame from, save the frame, and re-enable all of the tracks again. Custom filename formatting: I doubt that folks will be using the Save Frame feature to export the many images that adding custom formatting would be useful for. If someone wanted something like you describe, I'd recommend they create a new folder, name is with the current date, and then save the frames in there. The user has the ability to override the default saved frame filename in the dialog, too. If they do that once, for the next saved frame, they can click on the first one, which would prepopulate that filename and then adjust the frame number. Default save location: The default save location is supposed to be the same directory as the .osp file (for saved projects) or the user's home directory (for unsaved projects), just as you're asking. This should also be the same behavior as the
Once a frame is saved, that directory will be used for additional saved frames (or exported videos) - I know this is working. The user can specify any directory they want using the I'd prefer not to create directories automagically (where would they be created for unsaved projects) that the user may not want to use because we'd either have to clean them up or leave the clutter laying around. |
Default save location (for when a project has been saved) is now fixed. Icon is now right-justified and swapped out for a darker version. I left the light version icon in the PR since we may want to change from dark to light icons when the theme is changed. @peanutbutterandcrackers - Mind taking another look, sir? 🤞 |
I'm sorry it took me quite a while to test it out. Here are a few things that I think could be improved (just my humble opinion).
|
I've updated the icon to the camera-photo-symbolic icon.
Ack - I don't know why this has been such a pain. Probably because I'm multitasking with other stuff. 😞 I think I finally got it fixed.
Not an issue with my N3WWN dev branch (https://github.com/N3WWN/openshot-qt/tree/save_frame-Issue_893) - is that the branch that you're using the HEAD of? I just tested with taking multiple pictures and either dragging and dropping them from the
I just set mine to ` and it worked perfectly. I then changed it to Ctrl+Shift+F, restarted OpenShot and that worked perfectly, too.
Keeping track of the number of saved frames would add another value that needs to be maintained in the .osp file. What would the use case be for saving the same frame over and over where it would be much more efficient to have the file name autoincrement the # of saved frames along with the frame number? The juice has to be worth the squeeze. 🍋 😉 |
Awesome! The symbolic icon theme works great, I think. :) Yes, that is the branch I was at the HEAD of. Strange. It works well now on this Elementary OS 0.4.1 machine. It didn't on another Linux Mint 18.3 machine yesterday. Oh well. Either ways, nothing to worry about, then. :) True. I think it's okay to not have auto-increments in the file name, then. I just thought if someone would want to export multiple frames and would want to ffmpeg them all? I don't know. But it's okay to leave it out. :) This feature now also remembers the last place I saved the frame to, even if the project is saved. However, for the first frame, it defaults to the home directory - even for a saved project. Could you please make it default to the directory where the project file resides if the project has been saved? I'm sorry I've been quite nag. You're awesome, BTW. You're seriously awesome! 👍 |
Hmmm... very odd. Are you sure everything is building correctly and that you're in the right branch? I just checked my local copy and everything is committed locally, there are no files which are not checked in, the most recent PR commit matches my most recent local commit and here's my screen: If you don't exec OpenShot from the command line, please try that and add I think what is happening is that it can't load that svg file and uses a system default.
Oh, you mean like
I'm unable to replicate this... based on the other issues that you're having, you may not have a complete checkout of the current PR. I'd be willing to bet a beer that your local checkout is at a39e6cf while the path retention was fixed at d95336c and the icon was included at c1b0401 . All three were pushed yesterday, but the latter two commits were submitted after the first. |
I just checked this on my friend's 32 bit LXLE machine and it works really well. (And yes, OpenShot 2.x does have an i386 version. Awesome, right?) Just as it is supposed to work. Maybe that is the reason. I'll test this out on the same Linux Mint 18.3 machine once my friend comes around (I've been testing things out on a few of my friends' linux machines - different distros to test out on). I think this looks great! I will try to test this again on the same Linux Mint 18.3 machine and get back. Great work, good sir! 👍 You're awesome! 🏆 |
I don't know. For some reason it does look like that on this Linux Mint 18.3 machine. I even installed Also, if that is so, perhaps we should change the name to be something unique so that only the icon we have specified ourselves will be rendered? The symbolic icon matches really well with the rest of OpenShot. Not the one that Mint is currently using for some reason. |
Uff. Silly me. I did change the icon theme (from Linux Mint's Appearance Preferences) and it works rather well. All I had to do was change the theme. Thank you for your help, good sir! tips top hat :) |
@N3WWN - So, basically, it'll all great, good sir. Except, if you can, I think if the dialogue box of the screengrab pop-up looked more like the screenshot attached in my previous comment, it'd be even better. But that's just extra functionality now. I think this PR is good to be merged. 👍 Thank you for making OpenShot better! |
Thanks @peanutbutterandcrackers and @ferdnyc for testing this and looking into the issues! 😁 I think the AppImage has a bunch of broken icons, too, as I have 44 icon warnings when I launch OpenShot. Here's one of them:
I think fixing the icons across the board should be part of a different issue/PR. |
Use 48kHz as default audio samplerate
@ferdnyc Should this PR be targeting the develop branch now? |
Answered in this comment. (Short-short version: "Yes.") |
@@ -320,5 +320,6 @@ | |||
<file>Humanity/actions/16/locked.svg</file> | |||
<file>Humanity/actions/16/transform-move.png</file> | |||
<file>Humanity/actions/16/view-list-icons.png</file> | |||
<file>Humanity/actions/custom/camera-photo-symbolic.svg</file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, I just noticed... if you're updating the src/images/openshot.qrc
file, there's one more step necessary. That file is compiled into a binary format by a utility called pyrcc
(Actually pyrcc5
for PyQt5), which is what's read by OpenShot. So, as src/images/README
notes, you need to run the following command from that directory:
pyrcc5 openshot.qrc -o openshot_rc.py
And then you should check in the updates to openshot_rc.py
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done! 😁
src/windows/main_window.py
Outdated
@@ -1974,6 +2062,9 @@ def setup_toolbars(self): | |||
spacer.setSizePolicy(QSizePolicy.Expanding, QSizePolicy.Expanding) | |||
self.videoToolbar.addWidget(spacer) | |||
|
|||
# Other controls (right-justified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a terribly minor nit, but I'd change this to "right-aligned" instead of "right-justified", because to "justify" in the printing sense means "to line up evenly" — in other words, "right-justified" text isn't necessarily text that's slid all the way over to the right, but rather it's text that forms an even vertical right margin, as opposed to normal left-justified text which has a ragged right margin. You can't really "justify" a single horizontal line of icons, or anything.
Of course, you could claim that the exact same constraints apply to the word "aligned", and I'd be forced to admit that you've got me there, so feel free to just ignore me and I'll shut up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no particular argument either way, so I'm good with making the change. 😜
My PR #1772 will (eventually) whack those icon-fallback messages. In fact they were one of my primary motivators for doing it, and the very first message I removed. |
I wonder if 2.4.2.x bugfix series should incorporate feature PRs that introduce new (translate-able) strings. But since this is a rather small feature, perhaps we could send out another call for translation and add this in the bugfix release? Maybe we should append an 'a' or something to identify bug-fix releases that ship with some new feature. I'd really like some inputs from the BDFL @jonoomph though. I think, because of my comment, I should probably make my OpenShot Roadmap Proposal open to you all current core devs. Please do feel free to comment on the gist if you have some revisions/suggestions/criticisms etc. |
Honestly, I think at some point new strings have to go into the code, so that they're available to be translated. Translators work off of a list of translatable strings present in the source, so it's sort of a catch-22. Don't get me wrong, in the run-up to a release I definitely understand the need not to introduce any new translatable elements. But post-release, the earlier new strings go in, the more opportunity there is to get them translated. Now, that being said, it's probably a very good idea if we have some procedure defined for (a) tracking and (b) announcing the addition of new strings that require translation into the code, since presumably there are translators who could be informed of those events and asked if they'd very kindly help out with the needed translations. I mean, some of this is assumptions on my part, I really don't know how the process of getting OpenShot translated functions, from a practical standpoint. @jonoomph is definitely the expert we need on that front. But I'd suggest, for starters, creating a tag that gets applied to any PR that adds or modifies translatable strings in the code, since merging those PRs should be treated as a Bigger Deal than merging any other sort of code change. |
…-from-appimage Removing libdrm.so.2 from AppImage
…daily and release builds
…-fix-version Fixing GitLab to no longer run CI for tags, and fixing version # for …
…y adds file overwrite confirmation), reformat text to correctly utilize translation, use a more rational default filename for the saved fram, and replace dialog boxes with status bar updates (a first in OpenShot! *wink*)
… to the project current path or HOME_PATH every time a frame is grabbed
…t-justify the Save Frame button and enforce centering of the playback controls. Update default save frame filename from Frame%04d.png to Frame-%05d.png, which is more in line with image sequence exports. Fix default path of saved frames if the project has been saved (was defaulting to user home dir regardless of whether the project was saved or not).
af3e4bc
to
d2ec282
Compare
Closing this PR against |
This PR addresses the feature request made in Issue #893.
A new preview window button has been added (a camera) which will save the current preview frame (at full profile resolution, regardless of preview quality/scale settings) to the project's current directory or the user's home directory if the project has not yet been saved.
The ability for the user to add a keyboard shortcut has been included, but no keystrokes have been assigned by default.
Presently, the file type is hardcoded to PNG.
A dialog window is shown reporting where the frame was saved or that an error has occurred.