From b85aef2f33794eb6bd7fa1153f7c411ef14c5b7c Mon Sep 17 00:00:00 2001
From: Michael Gregorius <michael.gregorius.git@arcor.de>
Date: Sun, 19 Apr 2020 22:08:09 +0200
Subject: [PATCH] Code review changes

Move openContainingFolder to the end of the method block.

Adjust FileBrowserTreeWidget::contextMenuEvent to the coding conventions
and also make the code more readable by splitting up some conditions.

Add comments to clarify as to why the member m_contextMenuItem is set to
nullptr at the end of the execution of contextMenuEvent. Please note
that this implementation is not exception safe and should be changed in
the future, e.g. by passing the FileItem as a parameter of the slot.
---
 include/FileBrowser.h   |  2 +-
 src/gui/FileBrowser.cpp | 53 +++++++++++++++++++++++++----------------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/include/FileBrowser.h b/include/FileBrowser.h
index 0e8ce7bf3..d36eacf07 100644
--- a/include/FileBrowser.h
+++ b/include/FileBrowser.h
@@ -125,9 +125,9 @@ private slots:
 	void activateListItem( QTreeWidgetItem * item, int column );
 	void openInNewInstrumentTrackBBE( void );
 	void openInNewInstrumentTrackSE( void );
-	void openContainingFolder();
 	void sendToActiveInstrumentTrack( void );
 	void updateDirectory( QTreeWidgetItem * item );
+	void openContainingFolder();
 
 } ;
 
diff --git a/src/gui/FileBrowser.cpp b/src/gui/FileBrowser.cpp
index 52641a73d..29f25ec30 100644
--- a/src/gui/FileBrowser.cpp
+++ b/src/gui/FileBrowser.cpp
@@ -364,30 +364,41 @@ QList<QString> FileBrowserTreeWidget::expandedDirs( QTreeWidgetItem * item ) con
 
 void FileBrowserTreeWidget::contextMenuEvent(QContextMenuEvent * e )
 {
-	FileItem * f = dynamic_cast<FileItem *>( itemAt( e->pos() ) );
-	if( f != NULL && ( f->handling() == FileItem::LoadAsPreset ||
-				 f->handling() == FileItem::LoadByPlugin ) )
+	FileItem * f = dynamic_cast<FileItem *>(itemAt(e->pos()));
+	if (f == nullptr)
 	{
+		return;
+	}
+
+	if (f->handling() == FileItem::LoadAsPreset || f->handling() == FileItem::LoadByPlugin)
+	{
+		// Set the member to the current FileItem so that it is available during the
+		// execution of the slots of the context menu we are about to create and execute.
 		m_contextMenuItem = f;
-		QMenu contextMenu( this );
-		contextMenu.addAction( tr( "Send to active instrument-track" ),
-						this,
-					SLOT( sendToActiveInstrumentTrack() ) );
-		contextMenu.addAction( tr( "Open in new instrument-track/"
-								"Song Editor" ),
-						this,
-					SLOT( openInNewInstrumentTrackSE() ) );
-		contextMenu.addAction( tr( "Open in new instrument-track/"
-								"B+B Editor" ),
-						this,
-					SLOT( openInNewInstrumentTrackBBE() ) );
-		contextMenu.addSeparator();
-		contextMenu.addAction( QIcon(embed::getIconPixmap( "folder" )),
-					tr( "Open containing folder" ),
+
+		QMenu contextMenu(this);
+
+		contextMenu.addAction(tr("Send to active instrument-track"),
 					this,
-					SLOT( openContainingFolder() ) );
-		contextMenu.exec( e->globalPos() );
-		m_contextMenuItem = NULL;
+					SLOT(sendToActiveInstrumentTrack()));
+		contextMenu.addAction(tr("Open in new instrument-track/Song Editor"),
+					this,
+					SLOT(openInNewInstrumentTrackSE()));
+		contextMenu.addAction(tr("Open in new instrument-track/B+B Editor"),
+					this,
+					SLOT(openInNewInstrumentTrackBBE()));
+
+		contextMenu.addSeparator();
+
+		contextMenu.addAction(QIcon(embed::getIconPixmap("folder")),
+					tr("Open containing folder"),
+					this,
+					SLOT(openContainingFolder()));
+
+		contextMenu.exec(e->globalPos());
+
+		// The context menu has been executed so we can reset this member back to nullptr.
+		m_contextMenuItem = nullptr;
 	}
 }