Close all but this (fixes #266)

[dan-giddins]

A fix to #266
Functionally it works including shortcut keys!
Visually it looks like this

I spent quite a bit of time trying to get it to look right and I can't... (the point where removing the padding line actually adds a lot more padding was the point at which I gave up)

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.91%2Bg7aed59740-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.91%2Bg7aed59740-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.91%2Bg7aed597-linux-x86_64.AppImage (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.91%2Bg7aed59740-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://12389-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.91%2Bg7aed59740-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12389?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12387-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.91%2Bg7aed59740-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12387?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/qw99be504wvog2hw/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37533266"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dma41fh5gnqou9i2/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37533266"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12390-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.91%2Bg7aed597-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12390?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12386-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.91%2Bg7aed59740-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12386?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "66027746a4878650a0e19e5459b609e11f506de2"}

[tinkerlevu]

How about using the phrase Close all others or Close others ?

[PhysSong]

You should probably look into QMdiArea::subWindowList for manipulating MDI subwindows.

[dan-giddins]

@tinkerlevu done. I agree, 'close others' is a better name for it, but it still looks a bit broken. This is going to need a CSS expert to fix...

[dan-giddins]

You should probably look into QMdiArea::subWindowList for manipulating MDI subwindows.

@PhysSong given the functional nature of signals and slots, I think what I have now is a really neat functional solution. It works and is just an implementation of a generic functional solution to send a signal to multiple slots on multiple instances of an object. I guess I could have done a more procedural solution with QMdiArea::subWindowList, but I think this method better suits signals and slots, and would hazard a guess that its a little bit more efficient?

[IanCaio]

Just explaining the after-refactor tag: We've been planning a code refactor for a while and that depends on us clearing the PR log. For that reason, a while ago we started sort of a "PR-freeze": The PRs opened after that wouldn't be discarded, but they would have to be rebased after the refactor happened to fix any conflicts.

It's still possible that this PR will be merged before the refactor, but it's being tagged because I think it's only fair to merge a PR that was opened after the PR-freeze if we can merge all PRs that were opened after the PR-freeze. So it will all depend on how many PRs will be opened and how many reviewers and time left we have until the refactor begins.

Nothing to be majorly concerned about because even if this doesn't get merged before the refactor it will still be reviewed after it. Small PRs like this one shouldn't be too difficult to rebase, specially if the author accompanies the refactoring and does it incrementally as we advance with it.

[dan-giddins]

@IanCaio alright no worries! Generally I like to just merge the new master into my feature branch, but is it better to do a rebase? Either way, just let me know when I need to do that and I will do it :)
I don't think this needs to urgently get into master

[IanCaio]

@IanCaio alright no worries! Generally I like to just merge the new master into my feature branch, but is it better to do a rebase? Either way, just let me know when I need to do that and I will do it :)
I don't think this needs to urgently get into master

I personally prefer merging master too, other devs prefer rebasing. Either is fine IMHO 😄

[tinkerlevu]

done. I agree, 'close others' is a better name for it, but it still looks a bit broken. This is going to need a CSS expert to fix...

@dan-giddins ah well... it was worth a shot...

[dan-giddins]

@DomClark just a reminder about this PR

[zonkmachine]

Hi! I believe this was added via the merge with master. If you look further down the file there already is a
gui/modals/SetupDialog.cpp. The file was moved a while back. Did you do the merge all online? Does it compile locally?

Here is from the failing builds concerning this line but there seem to be others.

-- Configuring done
CMake Error at src/CMakeLists.txt:100 (ADD_LIBRARY):
  Cannot find source file:

    gui/SetupDialog.cpp

  Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
  .hxx .in .txx


-- Generating done
-- Build files have been written to: /__w/lmms/lmms/build
Error: Process completed with exit code 1.

[DomClark]

I agree with PhysSong's earlier comment.

Given the functional nature of signals and slots, I think what I have now is a really neat functional solution.

While I agree that signals and slots lend themselves to neat solutions, this is offset here by the need to maintain a singleton object through which to proxy the connections, which is otherwise unrelated to anything else in the code. This sort of hack indicates that signals and slots are not inherently suited to being used in the way they are here.

[It] is just an implementation of a generic functional solution to send a signal to multiple slots on multiple instances of an object.

A singleton is used here, which means that the signal is not sent just to multiple instances of an class, but in fact to all instances of that class. This is equivalent to maintaining a global list of all subwindows, and such mutable globals are often considered a code smell. It's not often that you want to do something to all instances of an class - usually, it's all instances within some context, and here the intention is to close all subwindows within a single MDI parent. This hints at a more appropriate place to maintain a collection of subwindows, and in fact this is done for you, as PhysSong pointed out.

Also, each instance of a class should be responsible for itself, and the objects it contains or owns, but not other instances that are unrelated except by type. Here, the MDI parent is responsible for its subwindows, and is where this implementation belongs. Be careful not to let the location of the menu item as seen by the user cause confusion when it comes to which object in the code is logically responsible for the action.

I guess I could have done a more procedural solution with QMdiArea::subWindowList, but I think this method better suits signals and slots

I don't think either method is more or less procedural - in both cases, you call a function to close all subwindows, which then iterates over the list of windows to close and in turn calls a function on each to close it. In the signal and slot case, this iteration is done within Qt, and while I'm in favour of offloading work to third-party code, in this case it comes with the downsides I've mentioned already.

[I] would hazard a guess that its a little bit more efficient?

In general, I would avoid guessing about performance benefits, and in this case both methods would be more than fast enough. I'm not convinced that the method in use here would be more efficient though - signal/slot connections are slower to dispatch than an ordinary function call.