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

Implementation of global messages #2598

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Jamulus.pro
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ FORMS_GUI = src/aboutdlgbase.ui \
HEADERS += src/buffer.h \
src/channel.h \
src/global.h \
src/messages.h \
src/protocol.h \
src/recorder/jamcontroller.h \
src/threadpool.h \
Expand Down Expand Up @@ -470,6 +471,7 @@ HEADERS_OPUS_X86 = libs/opus/celt/x86/celt_lpc_sse.h \
SOURCES += src/buffer.cpp \
src/channel.cpp \
src/main.cpp \
src/messages.cpp \
src/protocol.cpp \
src/recorder/jamcontroller.cpp \
src/server.cpp \
Expand Down
27 changes: 16 additions & 11 deletions src/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
#include <QDir>
#include <iostream>
#include "global.h"
#include "messages.h"
#ifndef HEADLESS
# include <QApplication>
# include <QMessageBox>
# include "serverdlg.h"
# ifndef SERVER_ONLY
# include "clientdlg.h"
Expand Down Expand Up @@ -901,6 +901,9 @@ int main ( int argc, char** argv )
bEnableIPv6,
nullptr );

// initialise message boxes
CMessages::init ( &ClientDlg, strClientName.isEmpty() ? QString ( APP_NAME ) : QString ( APP_NAME ) + " " + strClientName );

// show dialog
ClientDlg.show();
pApp->exec();
Expand All @@ -911,6 +914,9 @@ int main ( int argc, char** argv )
// only start application without using the GUI
qInfo() << qUtf8Printable ( GetVersionAndNameStr ( false ) );

// initialise message boxes
CMessages::init ( NULL, strClientName.isEmpty() ? QString ( APP_NAME ) : QString ( APP_NAME ) + " " + strClientName );

pApp->exec();
}
}
Expand Down Expand Up @@ -961,6 +967,9 @@ int main ( int argc, char** argv )
// GUI object for the server
CServerDlg ServerDlg ( &Server, &Settings, bStartMinimized, nullptr );

// initialise message boxes
CMessages::init ( &ServerDlg, strClientName.isEmpty() ? QString ( APP_NAME ) : QString ( APP_NAME ) + " " + strClientName );

// show dialog (if not the minimized flag is set)
if ( !bStartMinimized )
{
Expand All @@ -982,6 +991,9 @@ int main ( int argc, char** argv )
Server.SetDirectoryType ( AT_CUSTOM );
}

// initialise message boxes
CMessages::init ( NULL, strClientName.isEmpty() ? QString ( APP_NAME ) : QString ( APP_NAME ) + " " + strClientName );

pApp->exec();
}
}
Expand All @@ -990,17 +1002,10 @@ int main ( int argc, char** argv )
catch ( const CGenErr& generr )
{
// show generic error
#ifndef HEADLESS
if ( bUseGUI )
{
QMessageBox::critical ( nullptr, APP_NAME, generr.GetErrorText(), "Quit", nullptr );
ann0see marked this conversation as resolved.
Show resolved Hide resolved
}
else
CMessages::ShowError ( generr.GetErrorText() );
#ifdef HEADLESS
exit ( 1 );
#endif
{
qCritical() << qUtf8Printable ( QString ( "%1: %2" ).arg ( APP_NAME ).arg ( generr.GetErrorText() ) );
exit ( 1 );
}
}

#if defined( Q_OS_MACX )
Expand Down
58 changes: 58 additions & 0 deletions src/messages.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/******************************************************************************\
* Copyright (c) 2022
*
* Author(s):
* Peter Goderie (pgScorpio)
*
******************************************************************************
*
* This program is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation; either version 2 of the License, or (at your option) any later
* version.
*
* This program is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
* FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
* details.
*
* You should have received a copy of the GNU General Public License along with
* this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA
*
\******************************************************************************/

#include "messages.h"

tMainform* CMessages::pMainForm = NULL;
QString CMessages::strMainFormName;

/******************************************************************************\
* Message Boxes *
\******************************************************************************/
void CMessages::ShowError ( QString strError )
{
#ifndef HEADLESS
QMessageBox::critical ( pMainForm, strMainFormName + ": " + QObject::tr ( "Error" ), strError, QObject::tr ( "Ok" ), nullptr );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QMessageBox::critical ( pMainForm, strMainFormName + ": " + QObject::tr ( "Error" ), strError, QObject::tr ( "Ok" ), nullptr );
QMessageBox::critical ( pMainForm, strMainFormName + ": " + tr ( "Error" ), strError, tr ( "Ok" ), nullptr );

Is "Ok" really not an integrated button?

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks interesting. Are there any examples yet where the new message box class is a benefit already?

Well we first have to merge this one before we can modify the other messagebox calls of coarse...
But it has multiple benefits. From coding point of view showing a message becomes easier and more consistent, just CMessages::ShowError( 'text' ) and in headless mode the message will now show up on the terminal, so no actual need to check for HEADLESS defines.
From the users point of view it's especially of use for people running multiple instances, since the messagebox will popup on the instance generating the message and will have the 'clientname' in the title too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "Ok" really not an integrated button?

The standard messagebox has one button and here we just set the button text.
I don't like to depend on default values, since they might change. In this way I'm always sure what the result will be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks for the note here.

Concerning the QObject::tr() call: I think the code doesn't use it in other places? Should't we stay consistent and use tr()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like to depend on default values, since they might change. In this way I'm always sure what the result will be.

Yes, however the default values exist for a reason. Also they're translated automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerning the QObject::tr() call: I think the code doesn't use it in other places? Should't we stay consistent and use tr()

Yes, but since tr is a static QObject function it is only available in a QObject, so since here we are not a QOject here using QObject::tr here is actually the same as tr in a QObject.

#else
qCritical() << "Error: " << strError.toLocal8Bit().data();
#endif
}

void CMessages::ShowWarning ( QString strWarning )
{
#ifndef HEADLESS
QMessageBox::warning ( pMainForm, strMainFormName + ": " + QObject::tr ( "Warning" ), strWarning, QObject::tr ( "Ok" ), nullptr );
#else
qWarning() << "Warning: " << strWarning.toLocal8Bit().data();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why .toLocal8Bit()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is used everywhere, because the debug streams do not support unicode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok (although that's strange...)

#endif
}

void CMessages::ShowInfo ( QString strInfo )
{
#ifndef HEADLESS
QMessageBox::information ( pMainForm, strMainFormName + ": " + QObject::tr ( "Information" ), strInfo, QObject::tr ( "Ok" ), nullptr );
#else
qInfo() << "Info: " << strInfo.toLocal8Bit().data();
#endif
}
47 changes: 47 additions & 0 deletions src/messages.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#pragma once

/******************************************************************************\
* Copyright (c) 2022
* Author(s):
* Peter Goderie (pgScorpio)
*
* Use this static class to show basic Error, Warning and Info messages
*
* For own created special message boxes you should always use
* CMessages::MainForm() as parent parameter
* and CMessages::MainFormName() as base of the title parameter
\******************************************************************************/

#include <QString>
#ifndef HEADLESS
# include <QMessageBox>
# define tMainform QDialog
#else
# include <QDebug>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does QDebug include? Does it add bloat on a release build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QDebug includes what's neccesary for the cli streams.

# define tMainform void
#endif

// html text macro's (for use in message texts)
#define htmlBold( T ) "<b>" + T + "</b>"
#define htmlNewLine() "<br>"

class CMessages
{
protected:
static tMainform* pMainForm;
static QString strMainFormName;

public:
static void init ( tMainform* theMainForm, QString theMainFormName )
{
pMainForm = theMainForm;
strMainFormName = theMainFormName;
}

static tMainform* MainForm() { return pMainForm; }
static const QString& MainFormName() { return strMainFormName; }

static void ShowError ( QString strError );
static void ShowWarning ( QString strWarning );
static void ShowInfo ( QString strInfo );
};
1 change: 1 addition & 0 deletions src/util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ CAboutDlg::CAboutDlg ( QWidget* parent ) : CBaseDlg ( parent )
"<p>RobyDati (<a href=\"https://github.com/RobyDati\">RobyDati</a>)</p>"
"<p>Rob-NY (<a href=\"https://github.com/Rob-NY\">Rob-NY</a>)</p>"
"<p>Thai Pangsakulyanont (<a href=\"https://github.com/dtinth\">dtinth</a>)</p>"
"<p>Peter Goderie (<a href=\"https://github.com/pgScorpio\">pgScorpio</a>)</p>"
"<br>" +
tr ( "For details on the contributions check out the %1" )
.arg ( "<a href=\"https://github.com/jamulussoftware/jamulus/graphs/contributors\">" + tr ( "Github Contributors list" ) + "</a>." ) );
Expand Down