Ctrl+S now opens SaveAs dialog for freshly saving file

#2903 Merged at f34784b
Repository
mrunzzz
Branch
Issue#1665
Repository
osrf
Branch
default
Author
  1. Mrunmayi Mungekar
Reviewers
Description

To solve Issue#1665 The resulting behaviour is that Ctrl+S (or File->Save) will open the SaveAs Dialogue box if the file hasn't been saved before.

P.S. New at this. So please provide me suggestions and corrections on code style. Thank you.

Comments (10)

  1. Louise Poubel

    This works for me! I just have one comment about a cppcheck error.

    Also, it would be interesting to write a test for this if you're interested, but it's not required. The test would go into MainWindow_TEST, trigger save / save as, fill the browse dialog as needed, verify that the file was created, then delete the file. There is a similar example here; it's not exactly the same because save and save as have slightly different meanings there, but the way the action is triggered and the file is saved should be the same.

      1. Mrunmayi Mungekar author

        Actually yes @Louise Poubel ! The method of triggering the menu is varied here. Instead of Ctrl+Shift+S or simply clicking the option in the menu one uses Ctrl+S to trigger SaveAs. How does one incorporate the method of a trigger in the test?

        EDIT: Actually I think I got this. But as I have modified the Save function in this PR, wouldn't it be better to just modify the Save Test than to create another one for this?

        1. Louise Poubel

          Glad you figured it out! If there's already a save test, we can either change it or add another test next to it. Which test are you referring to, I don't see any under MainWindow_TEST.

          Let me know if you still need help triggering the actions from the test. One way would be by taking the action from the menu like in the ign-gui example above and calling trigger.

    1. Louise Poubel

      Thanks for merging! Here are the latest builds:

      • Ubuntu Xenial with GPU Build Status
      • Homebrew failed but it seems to have a different cause Build Status

      I'm good with the PR the way it is, thanks again! Approving!