Skip to content
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

Add file extension automatically for record plugin #303

Merged
merged 8 commits into from
Sep 16, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Aug 20, 2020

This PR automatically adds the user selected file extension to the file name entered by the user, so that they do not have to manually add the extension themselves.

Closes osrf/subt#362

Signed-off-by: John Shepherd [email protected]

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #303 into ign-gazebo2 will increase coverage by 24.26%.
The diff coverage is 76.33%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-gazebo2     #303       +/-   ##
================================================
+ Coverage        53.78%   78.05%   +24.26%     
================================================
  Files              121      183       +62     
  Lines             5838    10029     +4191     
================================================
+ Hits              3140     7828     +4688     
+ Misses            2698     2201      -497     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
include/ignition/gazebo/ServerConfig.hh 100.00% <ø> (ø)
...nclude/ignition/gazebo/components/Serialization.hh 61.29% <0.00%> (+61.29%) ⬆️
.../plugins/component_inspector/ComponentInspector.cc 9.41% <0.00%> (ø)
src/systems/breadcrumbs/Breadcrumbs.hh 100.00% <ø> (ø)
src/systems/diff_drive/DiffDrive.hh 100.00% <ø> (ø)
src/systems/joint_controller/JointController.hh 100.00% <ø> (ø)
...stems/joint_state_publisher/JointStatePublisher.hh 100.00% <ø> (ø)
src/systems/physics/Physics.cc 74.12% <ø> (+58.14%) ⬆️
... and 129 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e6c6ec...1f7b115. Read the comment docs.

@mjcarroll
Copy link
Contributor

As an alternative to this logic, the save dialog also has a property defaultSuffix that could be used:

https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#defaultSuffix-prop

@JShep1
Copy link
Author

JShep1 commented Aug 24, 2020

As an alternative to this logic, the save dialog also has a property defaultSuffix that could be used:

https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#defaultSuffix-prop

Thanks I'll check this out and see if I can condense the logic

@nkoenig
Copy link
Contributor

nkoenig commented Aug 25, 2020

As an alternative to this logic, the save dialog also has a property defaultSuffix that could be used:
https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#defaultSuffix-prop

Thanks I'll check this out and see if I can condense the logic

The might fix the case where the user enters the extension in the file dialog box, which results in <name>.mp4.mp4.

@JShep1
Copy link
Author

JShep1 commented Aug 25, 2020

The might fix the case where the user enters the extension in the file dialog box, which results in <name>.mp4.mp4.

I initially implemented this logic then hesitated. The condition that QML specifies for the defaultSuffix attribute is that it is appended if no other extension is specified. This could introduce an issue in which the user could improperly add .ogv to the end of their mp4 video title and then QML would allow it since it technically sees a proper file type extension.

I feel if we went this route we would want to compare the entered user extension to the user chosen video type in order to ensure a correct video extension. I realized this logic could become a lot more complicated than needed, and may introduce unexpected or undesired behavior (modifying the user entered extension, which I guess leads to how much we want to trust the user with what they're doing). All of this is why I simply went with appending the initially chosen video format. What do you think would be the best route here? @nkoenig

@JShep1
Copy link
Author

JShep1 commented Aug 25, 2020

As an alternative to this logic, the save dialog also has a property defaultSuffix that could be used:

https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#defaultSuffix-prop

Don't think we'll be able to do this, defaultSuffix was introduced in qt 5.10 and Bionic brings in 5.9.

@nkoenig
Copy link
Contributor

nkoenig commented Aug 31, 2020

The might fix the case where the user enters the extension in the file dialog box, which results in <name>.mp4.mp4.

I initially implemented this logic then hesitated. The condition that QML specifies for the defaultSuffix attribute is that it is appended if no other extension is specified. This could introduce an issue in which the user could improperly add .ogv to the end of their mp4 video title and then QML would allow it since it technically sees a proper file type extension.

I feel if we went this route we would want to compare the entered user extension to the user chosen video type in order to ensure a correct video extension. I realized this logic could become a lot more complicated than needed, and may introduce unexpected or undesired behavior (modifying the user entered extension, which I guess leads to how much we want to trust the user with what they're doing). All of this is why I simply went with appending the initially chosen video format. What do you think would be the best route here? @nkoenig

I think most applications will use whatever extension is specified, or automatically add an extension. Examples:

  1. User input: "test.mp4" --> Created file: "test.mp4"
  2. User input "test" --> Created file: "test.mp4"
  3. User input "test.garbage" --> Created file: "test.garbage"

@JShep1
Copy link
Author

JShep1 commented Aug 31, 2020

I think most applications will use whatever extension is specified, or automatically add an extension.

Implementation added, ready for another pass.

@chapulina chapulina merged commit 0af2c5f into ign-gazebo2 Sep 16, 2020
@chapulina chapulina deleted the jshep1/add_video_extension_automatically branch September 16, 2020 17:07
doisyg pushed a commit to wyca-robotics/ign-gazebo that referenced this pull request Dec 13, 2020
Signed-off-by: John Shepherd <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Signed-off-by: Guillaume Doisy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants