Fix for issue 43

[mcres]

Issue #43

This will filter the record menu and write the format to the file according to which button the user pushed (mp4 or ogv).

This is my first PR, I apologize if there are any obvious errors!

[mabelzhang]

Thanks for the PR! I tried it out. It works in general, but there's a few points in user friendliness that can be improved.

For example, if I select mp4 from the record menu, then in the drop-down list, I change my mind and select .ogv, type in test.ogv for the file name, the result is a file called test.ogv.mp4, a MPEG4 file. That's unexpected. I would have expected the file to come out as test.ogv. I think that logic can be added where you currently check urlHasFormat in VideoRecorder.cc.

A finer point is, if I choose *.mp4 in the drop-down list, and then type in file.ogv as the file name, which one do you respect? I don't know if there's a general rule for this. I just tried it out in Gimp, and they respect the one I type in, e.g. if I select jpg in drop-down list and type in file.png, then Gimp will save a PNG file.

[mcres]

You're right, I didn't notice that. Thanks for the suggestions! I'll work on that.

I think we should respect the typed name as well, independently of the button pushed in the record menu.
However, what happens when the user tries to save the file as something other than *.mp4 or *.ogv, for instance *.avi? Maybe we should respect that, even if the format doesn't exist at all? If I save a video as ignition.myOwnFormat, VLC is still able to open and play it.

From a user's point of view, that should probably be the behaviour. But I've realized that there could be a discrepancy between VideoRecorder::OnStart and VideoRecorder::OnSave as we could be recording in one format and saving in another one? It doesn't seem to be a problem, but I put the question here just in case.

[mcres]

With this commit, I save the file with the format specified in the drop-down list (which in turn is initially set depending on the selected button in the record menu, but can be changed).
If the user types any format, I respect that one.

This is the most intuitive behavior for me, but any comments are appreciated!

[mabelzhang]

discrepancy between VideoRecorder::OnStart and VideoRecorder::OnSave

Ohhh, I tried it out and I see what you mean. That is a problem. In that case, we cannot respect the user's input of extension (sorry for suggesting that), because the format we started with must be the file we save.

Is there a way to pre-populate the file name box with the extension OnStart was called with? I notice that it's usually populated with the name in a previous save, or blank with no extension. Having the extension there would be a better cue to the user that they are required to save it in that format.

Then we can revert to the original behavior of appending the extension to the end of the user input, regardless of what the user puts in. Except if the user already put the correct extension in the file name box, then we don't add the extension again.

If I save a video as ignition.myOwnFormat, VLC is still able to open and play it.

That doesn't mean it's a valid format. In Ubuntu, if you right-click on the file and go to Properties, then under the Video tab > General > Container, you will see the actual format of the file, regardless of what the extension is.

For example, I was able to save a .mp4 file whose actual Container says Ogg, and an .ogv file with Video > Codec H.264, which is a mp4 encoding. If I select mp4 and save a .avi file, it still also says Codec H.264. Any media player is able to look at the actual format, not the extension, to determine how to play it.

[mcres]

Then we can revert to the original behavior of appending the extension to the end of the user input, regardless of what the user puts in. Except if the user already put the correct extension in the file name box, then we don't add the extension again.

OK, let's do that. If the user writes fileName.avi, should that be converted to fileName.avi.mp4 or fileName.mp4? I guess the second one, right?

Is there a way to pre-populate the file name box with the extension OnStart was called with?

I looked at the FileDialog documentation, but I'm not sure how I can find out what's the exact Qt version Ignition is using to check the corresponding documentation.
For example, the defaultSuffix property could be useful but seems to belong to a more recent Qt version.
Another possibility would be using fileUrl, but it turns out it is a read-only property.

I didn't found much on Google, but I'll keep searching and see if something comes up.

[mabelzhang]

If the user writes fileName.avi, should that be converted to fileName.avi.mp4 or fileName.mp4? I guess the second one, right?

Let's do the first one, because the user might want to put a period somewhere in the file name, e.g. 2020.05.07, and we wouldn't want to deal with figuring out what they meant. In that example, we can't just strip everything after the . and append mp4, as it'd result in 2020.05.mp4.

what's the exact Qt version Ignition is using

The CMakeLists.txt shows it's Qt5 https://github.com/ignitionrobotics/ign-gazebo/blob/master/CMakeLists.txt#L80

If it's too hard then we can skip it... It's just a nice-to-have.

[mcres]

It seems that currently there's no way to preset the name field in FileDialog, or at least I couldn't find one... Maybe the defaultSuffix property can be helpful if Qt libraries are updated someday?

Right now, there's no problem when saving multiple times in the same format.
However, if the first time I write fileName.mp4 and save it, and next time I decide to save as .ogv, the preset name field of the file dialog will then be fileName.mp4. In that case, I could change from fileName.mp4 to fileName.ogv for the user, but it doesn't seem intuitive either.
So, in this situation I guess that I expect the user to change the format before saving.

[mabelzhang]

@iche033 @JShep1 Do you guys have input on the user interface for saving a recorded video? This PR adds the file extension to the file name, but the current behavior is a bit clunky. Maybe you know of a better way to access things in Qt. I would also like another pair of eyes on this PR before merging as I haven't done stuff with GUI.

[iche033]

we're using the Qt version on ubuntu bionic, which is 5.9.5, and I don't think that has support for the defaultSuffix property.

If the user tries to save in a different file extension than recorded, I think we can still respect the user's choice of filename but the encoding will not match, i.e. it'll be whatever format that was selected originally.

If we want to go one step further, we could pop up a confirmation dialog saying that the file extension in the user specified name does not match the encoding format. If user confirms, we save and close file dialog, else return to the file dialog and let user specify a different name.

[mcres]

@iche033 Thanks for the feedback!

If we want to go one step further, we could pop up a confirmation dialog saying that the file extension in the user specified name does not match the encoding format. If user confirms, we save and close file dialog, else return to the file dialog and let user specify a different name.

I think this is a good workaround, as we can't use the defaultSuffix property.
I'll work on that.

[mcres]

I've implemented the string logic in the .qml, so the other files remain the same as previous to the PR.
(Didn't know I could do that and I thought it was a good idea).

There's also the confirmation dialog suggested by @iche033 when file extension & video encoding don't match.

[mabelzhang]

Looks good to me. The mismatch check works for me. I have a 4k screen - not sure if that's different from yours, but the dialog looks fine on my screen.

Minor style comment.

Could you merge master into your branch so it has the latest?

[mabelzhang]

Last minor style comment.

Jenkins shows some style issues from another PR that we might as well fix here, they should be quick:
https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/3568/cppcheckResult/
The one about the header file is just a matter of reordering that line until that warning disappears.

You can run style tests locally with tools/code_check.sh and tools/clang_tidy.sh.

[mabelzhang]

No more new style errors and test failures. Approving.