Coupling between core and GUI

[M374LX]

Another issue related to code maintainability.

One way to make the code more maintainable is to make the core independent from the GUI, that is, keep low coupling.

The GUI should be allowed to use methods from the core (to both read and change its state), but the opposite should be avoided. In other words, instead of storing data in the GUI and allowing the core to read and change it (by means of methods, of course), data should be stored in the core and the GUI should read and change it. Data needed only by the GUI would be kept in the GUI, naturally.

However, the core is somewhat highly dependent from the GUI currently.

[badosu]

@M374LX Please see #908 for some context.

Even after decoupling, it would be desirable to achieve Real Time Safety, which is even more ambitious. See also #961 for more interesting stuff

[softrabbit]

However, the core is somewhat highly dependent from the GUI currently.

@M374LX, Any concrete sore spots you can point at?

I see Models everywhere, and they should fit the description "data being stored out of the GUI" pretty well, right?

[michaelgregorius]

@softrabbit Here's what a grep for "gui" in the core folder gives:

[lmms-dev]$ cd lmms/src/core/
[core]$ grep -r "gui"
midi/MidiOss.cpp:#include "gui_templates.h"                                                                                                                   
midi/MidiAlsaSeq.cpp:#include "gui_templates.h"                                                                                                               
midi/MidiWinMM.cpp:#include "gui_templates.h"                                                                                                                 
midi/MidiAlsaRaw.cpp:#include "gui_templates.h"                                                                                                               
Engine.cpp:     return gui != nullptr;                                                                                                                        
audio/AudioJack.cpp:#include "gui_templates.h"                                                                                                                
audio/AudioJack.cpp:            QMessageBox::information( gui->mainWindow(),                                                                                  
audio/AudioJack.cpp:            QMessageBox::information( gui->mainWindow(),                                                                                  
audio/AudioAlsa.cpp:#include "gui_templates.h"                                                                                                                
audio/AudioPulseAudio.cpp:#include "gui_templates.h"                                                                                                          
audio/AudioPortAudio.cpp:#include "gui_templates.h"                                                                                                           
audio/AudioSdl.cpp:#include "gui_templates.h"                                                                                                                 
audio/AudioOss.cpp:#include "gui_templates.h"                                                                                                                 
TrackContainer.cpp:                                             gui->mainWindow() );                                                                          
Track.cpp:#include "gui_templates.h"                                                                                                                          
Track.cpp:      if( m_trackView->trackContainerView() == gui->getBBEditor()->trackContainerView() )                                                           
Track.cpp:      if( m_trackView->trackContainerView() != gui->getBBEditor()->trackContainerView() )                                                           
Song.cpp:       if( gui && gui->getBBEditor() )                                                                                                               
Song.cpp:               gui->getBBEditor()->trackContainerView()->clearAllTracks();                                                                           
Song.cpp:       if( gui && gui->songEditor() )                                                                                                                
Song.cpp:               gui->songEditor()->m_editor->clearAllTracks();                                                                                        
Song.cpp:       if( gui && gui->fxMixerView() )                                                                                                               
Song.cpp:               gui->fxMixerView()->clear();                                                                                                          
Song.cpp:       if( gui && gui->automationEditor() )                                                                                                          
Song.cpp:               gui->automationEditor()->setCurrentPattern( NULL );                                                                                   
Song.cpp:       if( gui && gui->pianoRoll() )                                                                                                                 
Song.cpp:               gui->pianoRoll()->reset();                                                                                                            
Song.cpp:       if( gui && gui->getProjectNotes() )                                                                                                           
Song.cpp:               gui->getProjectNotes()->clear();                                                                                                      
Song.cpp:       if( gui->mainWindow() )                                                                                                                       
Song.cpp:               gui->mainWindow()->resetWindowTitle();
Song.cpp:       if( gui->mainWindow() )
Song.cpp:               gui->mainWindow()->resetWindowTitle();
Song.cpp:                       gui->fxMixerView()->refreshDisplay();
Song.cpp:                               if( node.nodeName() == gui->getControllerRackView()->nodeName() )
Song.cpp:                                       gui->getControllerRackView()->restoreState( node.toElement() );
Song.cpp:                               else if( node.nodeName() == gui->pianoRoll()->nodeName() )
Song.cpp:                                       gui->pianoRoll()->restoreState( node.toElement() );
Song.cpp:                               else if( node.nodeName() == gui->automationEditor()->m_editor->nodeName() )
Song.cpp:                                       gui->automationEditor()->m_editor->restoreState( node.toElement() );
Song.cpp:                               else if( node.nodeName() == gui->getProjectNotes()->nodeName() )
Song.cpp:                                        gui->getProjectNotes()->SerializingObject::restoreState( node.toElement() );
Song.cpp:       if( gui && gui->mainWindow() )
Song.cpp:               gui->mainWindow()->resetWindowTitle();
Song.cpp:               gui->getControllerRackView()->saveState( dataFile, dataFile.content() );
Song.cpp:               gui->pianoRoll()->saveState( dataFile, dataFile.content() );
Song.cpp:               gui->automationEditor()->m_editor->saveState( dataFile, dataFile.content() );
Song.cpp:               gui->getProjectNotes()->SerializingObject::saveState( dataFile, dataFile.content() );
Song.cpp:// save current song and update the gui
Song.cpp:bool Song::guiSaveProject()
Song.cpp:               gui->mainWindow()->resetWindowTitle();
Song.cpp:bool Song::guiSaveProjectAs( const QString & _file_name )
Song.cpp:       if( guiSaveProject() == false )
Song.cpp:               QMessageBox::information( gui->mainWindow(),
Song.cpp:       FileDialog efd( gui->mainWindow() );
Song.cpp:               ExportProjectDialog epd( exportFileName, gui->mainWindow(), multiExport );
Song.cpp:               QMessageBox::information( gui->mainWindow(),
Song.cpp:       FileDialog efd( gui->mainWindow() );
Song.cpp:               if( Engine::hasGUI() && gui->mainWindow() &&
Song.cpp:                       QThread::currentThread() == gui->mainWindow()->thread() )
Song.cpp:                       gui->mainWindow()->resetWindowTitle();
main.cpp:                       QMessageBox::question( gui->mainWindow(), MainWindow::tr( "Project recovery" ),
main.cpp:                       gui->mainWindow()->show();
main.cpp:                               gui->mainWindow()->showMaximized();
main.cpp:                       gui->mainWindow()->show();
main.cpp:                               gui->mainWindow()->showMaximized();
main.cpp:                       gui->mainWindow()->show();
main.cpp:                               gui->mainWindow()->showMaximized();
Mixer.cpp:              gui->pianoRoll()->isRecording() == true &&

It just a few lines but these contain audio drivers showing dialogs and the Song class accessing GUI classes left and right. :)

Possible solutions might be:

• Add signals to the core classes which trigger changes in the GUI. However, this solution might clash with the goal of real-time safety at a later point in time.
• Use queues to communicate events in the core classes to the GUI main thread.

In my opinion decoupling the core from the GUI should also imply being able to compile the core independently of the GUI and that the core should not have to link QtGui but only QtCore and similar libraries.

[M374LX]

@softrabbit
Example spots include the Song class, mainly in the methods related to loading and saving the project. This class also accesses TimeLineWidget do determine whether looping is enabled and to find the looping points.

Mixer accesses the PianoRoll to determine whether notes are being recorded and play the metronome samples (I suggested an update on #1828, but now I think a better idea is to move the handling of the metronome to Song. Thus, I will update the PR soon).

Many other core classes show message boxes.

Being able to compile the core without the GUI, as suggested by @michaelgregorius, would improve the program's testability.

[softrabbit]

Let's see... the audio and MIDI drivers provide setup widgets, good to know. But I'll save my thoughts on that for another issue.

audio/AudioJack.cpp: QMessageBox::information( gui->mainWindow()

Wasn't some error collection or logging feature introduced recently, and could that be extended to handle urgent messages in a gui-independent way as well?

Looks like it's only to collect errors when loading a Song. Forget I asked :)

@michaelgregorius:

Add signals to the core classes which trigger changes in the GUI. However, this solution might clash with the goal of real-time safety at a later point in time.

In Song at least, signals should be fine as file operations are RT unsafe anyway, right? And that class looks like a pretty big offender so far.

@M374LX:

Being able to compile the core without the GUI, as suggested by @michaelgregorius, would improve the program's testability.

Sure, if the GUI code gets in the way. ISTR some discussion on the subject, but was there ever a decision on which testing framework to use?

[michaelgregorius]

Add signals to the core classes which trigger changes in the GUI. However, this solution might clash with the goal of real-time safety at a later point in time.

In Song at least, signals should be fine as file operations are RT unsafe anyway, right? And that class looks like a pretty big offender so far.

File operation do not necessarily have to be RT unsafe. I think the coarse recipe for these cases is as follows. Let's say you want to switch a sample that's played in a sampler.

1. Each time the RT thread is executed it uses the buffer with the soon-to-be old sample.
2. The GUI triggers the loading of a different sample.
3. The new sample is loaded in a thread which is not the RT thread and is stored in a new buffer.
4. Once the buffer is prepared a command is queued for the RT thread.
5. The RT thread executes the command which just switches the buffers.
6. The RT thread queues a command so that the old buffer can be release in a non RT thread.
7. Each time the RT thread is executed now it will use the new sample.

Being able to compile the core without the GUI, as suggested by @michaelgregorius, would improve the program's testability.

Sure, if the GUI code gets in the way. ISTR some discussion on the subject, but was there ever a decision on which testing framework to use?

The GUI code will likely get in the way quite fast. Imagine you wanted to write unit tests for the Song class. You'd either have to have all the dialogs available during the test execution or you'd have to write code as currently seen in the Song class which checks a global variable that indicates whether the GUI is available which is quite clunky.

[lukas-w]

Closing this because it's a design principle and not a clearly defined, achievable goal and therefore doesn't serve any real purpose on this tracker.