HaikuArchives / ffmpegGUI

GUI for FFmpeg
MIT License
23 stars 10 forks source link

Organize container formats, audio and video codecs in containers #89

Closed andimachovec closed 1 year ago

andimachovec commented 1 year ago

New classes ContainerOption and CodecOption are introduced. Format and codec options are kept in std::vector containers of these classes. This gives us the possibility to use descriptive names in the popup menus instead of the ffmpeg option strings. The available options are still managed manually, because fetching them from ffmpeg would give us too many of them.

Problem: currently messes up the layout when codec options are switched away from "1:1" copy. @humdingerb : Could you please take a look at this and maybe make some suggestions ;-)

humdingerb commented 1 year ago

As long as the codec/format descriptions are this elaborate, it'll always spoil the layout as they are just sooo long. :) I think it should be possible to keep the abbreviation (avi, mpeg, FLAC...) as label when the pop-up is closed and only show the long description if the pop-up is clicked and all items are shown for choosing.

andimachovec commented 1 year ago

I think it should be possible to keep the abbreviation (avi, mpeg, FLAC...) as label when the pop-up is closed and only show the long description if the pop-up is clicked and all items are shown for choosing.

That's a novel approach I certainly didn't think of ;-) Shouldn't be too hard to implement, we'd probably need to subclass either BPopUpMenu or BMenuField (or both), somehow catch when the menu gets opened and then change the values. That said, for simplicity's sake, shouldn't we just shorten the descriptions instead to make them work with the layout?

humdingerb commented 1 year ago

I would have thought it were easier. I imagined to give the "labelFromMarked" bool a false when creating the BPopUpMenu. For example:

popup = new BPopUpMenu("PopUpName", false, false);
popup->SetRadioMode(true);

Now the popup always shows "PopUpName" instead of the marked item label. Unfortunately, I haven't found out how to change that name... :) If that were possible, it'd be straight forward: When receiving the BMessage that the menu was invoked, get the marked item's extension and set it as popup menu name. Maybe you can figure that out...

I don't think there's a another equally nice solution. Anything longer than the 3 or 4 letter extension will make the window really wide, considering we have a video and an audio codec label. to fit there.

humdingerb commented 1 year ago

No time to investigate further ATM. But it seems BMenu's Superitem() does what's needed.

BMenuItem* item = popup->Superitem();
item->SetLabel("Extension");
andimachovec commented 1 year ago

I've added some code to calculate the needed minimum size for the popups. Works nicely and is less work (and probably less confusing for the user) than switching the menu label strings.

Not ready to merge yet, I've still to add some formats and disable video encoding if an audio-only format is selected

humdingerb commented 1 year ago

Would you, just to humour me, have a look at my shortpops branch.

Just try it out for a bit. I don't think it's confusing to have a shortenedlabel for the selected format/codec and the additional full description when opening the popup menu. Also, without using just the extension for the container format, we can never get the popup into the output file row above, because it'll be much to wide.

Here is the zipped up binary, if you don't want to go through the hassle to check out and compile: ffmpegGUI.zip

andimachovec commented 1 year ago

Would you, just to humour me, have a look at my shortpops branch.

Of course, you can always count on my curiosity ;-) Thanks for the zip but it's not necessary. I have a clone of your repo on my dev machine, I'm actually faster checking out your branch than expanding the zip I guess...

andimachovec commented 1 year ago

OK, here we go. It's an interesting approach, and not as horribly confusing as I imagined it to be ;-). Also much simpler to implement than I had thought. Still, my vote goes to the version with the long labels that don't change.

humdingerb commented 1 year ago

Still, my vote goes to the version with the long labels that don't change.

That's regrettable. I thought it was a neat way to avoid wasting space and only make additional information available when it counts: while choosing the video/audio codecs.

I hope you will consider using extensions-only for the container file-format. avi, mpeg, mkv etc are very well known even among casual AV users. We could save a lot of GUI estate if the now single pop-up menu were moved to the end of the output file text box...

andimachovec commented 1 year ago

That's regrettable.

That's only my view on it, I don't have a problem at all if we go with your solution. I we do it would be great if we could merge in my code to handle audio only container formats.

humdingerb commented 1 year ago

Let's have you finish this PR first. Then we see how it look&feels and maybe push my change as a single commit that can easily be reverted, just tocontrast and compare.

Though, after merging this PR, I'd reall like to do some code style cleaning first. Or we'll end up doing it /after/ all our work... :)

andimachovec commented 1 year ago

Let's have you finish this PR first. Then we see how it look&feels and maybe push my change as a single commit that can easily be reverted, just tocontrast and compare.

Alright, works for me ;-)

Though, after merging this PR, I'd reall like to do some code style cleaning first. Or we'll end up doing it /after/ all our work... :)

Yeah, fully agree with you. We got carried away a little bit ;-) But on the positive side you've got to use the motivation as long as it it`s there :-)

andimachovec commented 1 year ago

Conflicts resolved, ready to merge. Btw I like where the file format pop is positioned now.

humdingerb commented 1 year ago

Let's merge then and I get on the code style...

andimachovec commented 1 year ago

Let's merge then and I get on the code style...

Great, thank you. If you need help with the coding style cleanup just let me know.

humdingerb commented 1 year ago

You can have a look at #90 and we take it from there.