HaikuArchives / ffmpegGUI

GUI for FFmpeg
MIT License
23 stars 10 forks source link

Rework MainWindow::_BuildLine() to respect manual changes to the command line #122

Closed andimachovec closed 1 year ago

andimachovec commented 1 year ago

The code is still kind of raw and quite possibly contains several bugs. Since it involves parsing user edited content it will require some extensive testing. Nevertheless, the goal should not be to make it completely idiot proof, but to be able to handle reasonable changes and additions to the ffmpeg commandline by the user.

Happy testing! :-)

humdingerb commented 1 year ago

From a few quick tests, it works just fine... Did you rebase master recently? I think some of my fixes WRT to BSpinner value setting aren't in... (even if these fixes didn't work in all cases...)

andimachovec commented 1 year ago

Just the same few coding style corrections around if-else-blocks over and over. I know, you love it... :P

Don't worry, I'm going to clear those up before merging ;-) Especially the brackets around single line if blocks occur quite easily, sometimes lines get added or removed. Cleaning this up at every change while working on something is very distracting.

humdingerb commented 1 year ago

👍

andimachovec commented 1 year ago

Did you rebase master recently?

The branch was created right after the 1.2 release iirc. I didn't rebase after that. Shouldn't cause any merge conflicts, my changes are confined to _BuildLine() and a few newly created helper functions.

I'll do a little more testing, clean up the code and we should be ready for merge.

andimachovec commented 1 year ago

I cleaned up the code from hopefully any coding style violations. Further testing revealed no errors, just some limitations. For example we can't currently set a parameter multiple times, although it's no problem when the user does it manually. Might become relevant if/when we add features to handle metadata. But it can be enhanced later if we need to. I'd say it's ready to merge.

andimachovec commented 1 year ago

Well, that took a bit longer than expected, sorry. Too much outside activity going on at this time of the year ;-) Really hope that I've fixed all style violations now. But if not, please don't hesitate to point them out. I used the checkstyle.py tool which isn't really that smart (it doesn't get the difference between a mathematical operator used in C++ code vs. the character for the operator used in a string, among other things), but it did help find out a few more style errors. I still think there are a few rules among our style guidelines that are, let's say a bit unfortunate. Especially having to start the else statement on the same line than the closing bracket of the above if block makes quickly getting a mental picture of nested if/else blocks very hard for me. But as I said this discussion really belongs on the dev mailing list.

humdingerb commented 1 year ago

I sympethize about the weather. Pretty much the perfect temperature now. I read the 30+° that is bound to arrive some time...

Thanks for the style fixes. Once you've been reading code in that style for 20 years you've got used to it... :)

andimachovec commented 1 year ago

Once you've been reading code in that style for 20 years you've got used to it... :)

Yeah, that´s comforting to know that. I´ll keep on keepin´on :-) Thanks for merging 👍