HaikuArchives / ffmpegGUI

GUI for FFmpeg
MIT License
23 stars 10 forks source link

Edit jobs #113

Closed humdingerb closed 1 year ago

humdingerb commented 1 year ago

One thing missing: We're currently not archiving and upon editing unarchiving the ffmpeg commandline. This commandline could have been amended with custom parameters by the user.

Unfortunately assigning any setting will trigger a BMessage for the set widget, which in turn triggers a BuildLine() which will overwrite the ffmpeg commandline and therefore the user customization...

It would be possible to set each widgets ModificationMessage to NULL while setting its value and re-set the original message when done (as alredy done for the fSourceTextControl to avoid _GetMediaInfo()), but doing that for every widget seems stupid...

Short of creating an extra textcontrol for custom parameters, I'm out of ideas.

humdingerb commented 1 year ago

Short of creating an extra textcontrol for custom parameters, I'm out of ideas.

The more I think about it, the better this idea sounds. After all, any change of any widget will _BuildLine() and overwrite the custom parameters. That's not just an issue when editing a job.

I made a quick mockup:

mockup

Acceptable? What's your thoughts @andimachovec ?

humdingerb commented 1 year ago

@andimachovec , you may have seen https://discuss.haiku-os.org/t/how-to-disable-modification-messages/13200/10

The proposed solution of using timestamps to not trigger the BuildLine() does work. But it doesn't solve the general issue that with the next click on any widget, the preserved commandline is overwritten, and the custom options lost. So, the above proposal still stands.

andimachovec commented 1 year ago

you may have seen https://discuss.haiku-os.org/t/how-to-disable-modification-messages/13200/10

Yep, I just commented there with my proposed solution for the BSpinner invocation problem.

Unfortunately assigning any setting will trigger a BMessage for the set widget, which in turn triggers a BuildLine() which will overwrite the ffmpeg commandline and therefore the user customization...

There's also a different approach to this problem, not yet sure if it's the better one. Let's focus on BuildLine() instead of the invocation of it. BuildLine() currently has a more or less all or nothing approach, we could make it a little more fine grained. Instead of building a new commandline each time we would have to check what's already in the command string and adjust the parameters accordingly. And leave everything else alone. With this implemented correctly, the controls can trigger BuildLine() all they want and we still preserve changes done by the user. If you want I can have a go at this over the next few days.

humdingerb commented 1 year ago

Yep, I just commented there with my proposed solution for the BSpinner invocation problem.

Read. I did have a quick look-see, but I seem to be too lame to make it work... Can I burden you with that? :) One thing, though: I guess we'll need a SetValue() version with a second parameter, bool invoke = true. We do need notifications with normal operation, i.e. when the user clicks around. Only when we apply the settings of a job that got send back from editing, we don't. And maybe in the future, should ffmpegGUI gain "profiles", those could be applied as a re jobs now.

If you want I can have a go at this over the next few days.

With pleasure. Can I merge the jobs PR before you start that endeavour? (And your crop PR comes before that, I guess.)

andimachovec commented 1 year ago

Can I merge the jobs PR before you start that endeavour? (And your crop PR comes before that, I guess.)

Of course. I´ll also finish the crop PR first.

Read. I did have a quick look-see, but I seem to be too lame to make it work... Can I burden you with that? :)

Of course, I´ll do it. You´ll see it´s not very complicated ;-)

humdingerb commented 1 year ago

Of course, I´ll do it. You´ll see it´s not very complicated ;-)

I'll watc and learn. :)

humdingerb commented 1 year ago

@andimachovec : This morning, just waking up, I had the idea that you didn't mean just overriding just SetValue() and replicating all its code. I now override SetValue() with an additional bool parameter to signal not to Invoke(). Plus I overrode Invoke() to respect that parameter. Have a look at #114 . Is that what you intended as well, or is there an even better solution?

andimachovec commented 1 year ago

Have a look at #114 . Is that what you intended as well, or is there an even better solution?

Your solution uses even less code than mine would have, by overriding Invoke() as well. :-) I'm not sure about adding an additional parameter to an overridden function, please see my comment over at #114

humdingerb commented 1 year ago

I've updated the PR, also to address small changes the crop-preview PR effected. @andimachovec, give it little spin when you find the time. Thanks!

andimachovec commented 1 year ago

Tonight I'm out and about playing music...I'll try out the PR tomorrow morning 😀

andimachovec commented 1 year ago

Alrighty, been playing around with the job edit feature a bit. I have to admit I found it quite confusing at first. When the job disappeared from job manager after selecting edit job from the menu I thought it was a bug. Then I realised the settings of the job were loaded into the main window (which I agree makes sense, there`s no need for yet another edit window). After realising this, everything worked fine. Would be interesting to have some more people test this and see if they're confused as well or if it's only me ;-) Anyway, you're usually way more adept on UI and usability matters than I am, so I'll leave it to you if want to keep it this way or change it. Besides that, I don't have a better solution at hand.

humdingerb commented 1 year ago

Then I realised the settings of the job were loaded into the main window (which I agree makes sense, there`s no need for yet another edit window).

I'm aware of that issue. As you, I didn't wantto introuce a whole new edit window for this. I contemplated sending the job back to the main window in a special edit-mode that would auto-update the job in the jobs window. But how to cancel an edit or start a new job from there. It seems to make things even more complicated...

The MainWindow pops up tot he top while the job vanishes from the job window. That's one indication that the job is being edited and removed from the jobs list. To get it back into the list, you'll need to re-add it as new job. I'm adding a Help file that'll explain it for the few people reading documentation... :) Would you say renaming "Edit job" to "Send back to edit" would make things better?

andimachovec commented 1 year ago

Would you say renaming "Edit job" to "Send back to edit" would make things better?

No, I don't think so. Like I said, I don't have any ideas for improvement in this case. I'd say let`s merge it as it is. It isn't a central feature of the program, and we can still change it later if we come up with a better solution.

humdingerb commented 1 year ago

Alright then. Merge!