Implementation of global messages #2598

pgScorpio · 2022-04-14T20:56:07Z

Short description of changes
New implementation of the messageboxes part of #2583

CHANGELOG: Messageboxes will show up related to the instance instead of at system default position and they will have 'clientname' in the window title. In headless mode the messages will appear on the terminal.

Context: Fixes an issue?

General improvement and preparation for sound-redesign.

Does this change need documentation? What needs to be documented and how?

No documentation changes

Status of this Pull Request

Waiting on review

What is missing until this pull request can be merged?

Review

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

The previous logic used eval to dynamically create variables. This is hard to follow. Instead, explicitly name the relevant two variables. Also, introduce readable variable names. Related: jamulussoftware#2474

Some suggestions do not apply. For those, a comment is added to disable the check and explain why shellcheck cannot properly judge it. Related: jamulussoftware#2474

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

ann0see · 2022-04-15T09:36:58Z

Great! Thank you!

ann0see · 2022-04-15T09:40:49Z

Could you please add a CHANGELOG entry in your PR description (and a short reasoning why you need this + advantages e.g uniform way to show error messages in future)

ann0see

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

ann0see · 2022-04-15T09:45:02Z

src/messages.cpp

+void CMessages::ShowError ( QString strError )
+{
+#ifndef HEADLESS
+    QMessageBox::critical ( pMainForm, strMainFormName + ": " + QObject::tr ( "Error" ), strError, QObject::tr ( "Ok" ), nullptr );


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?

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.

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.

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()

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.

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.

pgScorpio · 2022-04-15T11:34:57Z

Could you please add a CHANGELOG entry in your PR description (and a short reasoning why you need this + advantages e.g uniform way to show error messages in future)

Done. Please let me know if it's ok this way.

pgScorpio · 2022-04-15T11:56:53Z

Questions:

Do we want a "Jamulus [clientname]" prefix to appear on the debug steams too?
(Personally I think this might be usefull.)

Would it be useful to be able to set the button text from the "ShowXxxx" calls (optional parameter defaulting to "Ok")?
(Personally I think we shouldn't for consistency, since different text's might give the expectation of different behavior.)

src/main.cpp

hoffie

I think the UI abstraction makes sense, as it saves passing the parent window and the title all over again.
I'm less sure about the headless stuff. My gut feeling is that there are not that many instances where there's duplicated error handling for UI/CLI stuff, but I might be wrong. I think the CLI logging handling should use some reworking as well, so this might cover it. But maybe we should do that on its own as the first step and see if it makes sense to merge that later? My main concern is that if we replaced all qWarning/qInfo calls with the new abstraction, we might start showing solely informational messages as dialogs accidentially?

I think it would be good to have @pljones's judgement here first.

PR note: It seems like this PR adds and includes the infrastructure (src/messages) and updates one caller place. If we decide to add it, it would make sense to update all callers in the same go (separate commit, but same PR).

(Maybe there was some confusion about that in the other PRs regarding what to include. A PR should do one thing. If it aims to improve the message box implementation, it should include all of that, even if that touches lots of different files. It's just that all those changes should be related to the same specific goal.)

src/main.cpp

ann0see · 2022-04-16T20:15:00Z

src/messages.cpp

+#ifndef HEADLESS
+    QMessageBox::warning ( pMainForm, strMainFormName + ": " + QObject::tr ( "Warning" ), strWarning, QObject::tr ( "Ok" ), nullptr );
+#else
+    qWarning() << "Warning: " << strWarning.toLocal8Bit().data();


Why .toLocal8Bit()?

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

Ok (although that's strange...)

ann0see · 2022-04-16T20:16:07Z

src/messages.h

+#    include <QMessageBox>
+#    define tMainform QDialog
+#else
+#    include <QDebug>


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

QDebug includes what's neccesary for the cli streams.

ann0see · 2022-04-16T20:17:41Z

My main concern is that if we replaced all qWarning/qInfo calls with the new abstraction, we might start showing solely informational messages as dialogs accidentially?

I mean you could also add a boolean parameter to only show it via CLI if needed.

ann0see · 2022-04-16T20:18:40Z

Would it be useful to be able to set the button text from the "ShowXxxx" calls (optional parameter defaulting to "Ok")?

Overload the methods to add the ability to do that?

pgScorpio · 2022-04-16T21:40:04Z

I'm less sure about the headless stuff. My gut feeling is that there are not that many instances where there's duplicated error handling for UI/CLI stuff, but I might be wrong.

There are several places places in the code with ifndef HEADLESS QMessage #else qStream #endif

The QMessageBox class would make those ifdefs unnecessary.
Also there are some places with only ifndef HEADLESS QMessage #endif where a message in headless mode would be useful too.
So my idea is that only if you explicitly don't want the message on the terminal in headless mode you should now use an ifdef, since I think there are very few occasions where you would not want to show errors/warnings/info in headless mode if there is a good reason to show them in ui mode.

My main concern is that if we replaced all qWarning/qInfo calls with the new abstraction, we might start showing solely informational messages as dialogs accidentially?

I'm not sure if I really understand this remark. But we have Error, Warning and Info messageboxes. We would just replace all QMessagebox::xxxx calls with the matching CMessageBox::ShowXxxx call.

PR note: It seems like this PR adds and includes the infrastructure (src/messages) and updates one caller place. If we decide to add it, it would make sense to update all callers in the same go (separate commit, but same PR).

Sure !, but I did that in the other PR and I was requested NOT to do that again this PR but in a separate PR after this one is merged....

(Maybe there was some confusion about that in the other PRs regarding what to include. A PR should do one thing. If it aims to improve the message box implementation, it should include all of that, even if that touches lots of different files. It's just that all those changes should be related to the same specific goal.)

So I think there is still some difference in opinion on that.
Just let me know what we agree upon, new commit or new PR doesn't make that much of a difference to me.

pgScorpio · 2022-04-16T22:06:52Z

Would it be useful to be able to set the button text from the "ShowXxxx" calls (optional parameter defaulting to "Ok")?

Overload the methods to add the ability to do that?

There are several ways to do it, but as said another button text would only seem useful to me if we take specific actions after the message is shown, so that would need another kind of (modal) message box that waits for the button to be pressed.
So I would rather implement those kind of message boxes too and those would then have a (optional?) button text parameter.
But AFAIK there are just one or two places in the current code where this would be applicable. (and one occurrence would be disappearing after the sound redesign anyway.)

Now translating html text to plain text before sending it to console.

ShowXxxWait messageboxes will wait for the/a button to be pressed in gui mode. ShowXxxWait messageboxes also have an optional 'abort' button and a default selection. In HEADLESS mode there is NO wait, and the default selection is returned.

Also added catching of generic exceptions withusing a ShowErrorWait

Can't use QTextEdit in messages Unused variables in main

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

Now translating html text to plain text before sending it to console.

ShowXxxWait messageboxes will wait for the/a button to be pressed in gui mode. ShowXxxWait messageboxes also have an optional 'abort' button and a default selection. In HEADLESS mode there is NO wait, and the default selection is returned.

Also added catching of generic exceptions withusing a ShowErrorWait

Can't use QTextEdit in messages Unused variables in main

…lobal-messages

ann0see · 2023-07-01T19:44:14Z

I think the idea is still valuable. However to keep the PR view clean, I'll close this for now.

pgScorpio and others added 6 commits April 14, 2022 21:47

Adding global message boxes

3d242d6

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

Adding global message boxes

eee5c8b

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

Bugfix headless

e92fd96

mac/deploy_may.sh: Replace eval magic by explicit variables

7a1fffb

The previous logic used eval to dynamically create variables. This is hard to follow. Instead, explicitly name the relevant two variables. Also, introduce readable variable names. Related: jamulussoftware#2474

Scripts: Apply shellcheck suggestions

00924c3

Some suggestions do not apply. For those, a comment is added to disable the check and explain why shellcheck cannot properly judge it. Related: jamulussoftware#2474

Adding global message boxes

a441d5c

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

ann0see requested review from hoffie and pljones April 15, 2022 09:37

ann0see added the refactoring Non-behavioural changes, Code cleanup label Apr 15, 2022

ann0see added this to Triage in Tracking (old) via automation Apr 15, 2022

ann0see reviewed Apr 15, 2022

View reviewed changes

pgScorpio commented Apr 15, 2022

View reviewed changes

src/main.cpp Show resolved Hide resolved

Update from vm changes on origin

ca933c1

hoffie mentioned this pull request Apr 16, 2022

Global MsgBoxes and Commandline #2583

Closed

5 tasks

hoffie reviewed Apr 16, 2022

View reviewed changes

ann0see reviewed Apr 16, 2022

View reviewed changes

ann0see moved this from Triage to Waiting on Team in Tracking (old) Apr 16, 2022

ann0see mentioned this pull request Apr 22, 2022

Logging: Make log messages consistent #1464

Open

pgScorpio mentioned this pull request Apr 28, 2022

Adding global commandline parameters #2599

Draft

5 tasks

pgScorpio added 3 commits April 30, 2022 01:26

No html output to console.

cb9a5e6

Now translating html text to plain text before sending it to console.

Added ShowXxxWait messageboxes.

424f667

ShowXxxWait messageboxes will wait for the/a button to be pressed in gui mode. ShowXxxWait messageboxes also have an optional 'abort' button and a default selection. In HEADLESS mode there is NO wait, and the default selection is returned.

Use ShowErrorWait in main

5837a93

Also added catching of generic exceptions withusing a ShowErrorWait

pgScorpio added 8 commits May 1, 2022 00:05

Fixing HEADLESS build...

3ff4ad8

Can't use QTextEdit in messages Unused variables in main

rebase to upstream

cebc2b4

Adding global message boxes

dca5145

Added files for global messageboxes with the main dialog as parent and with a standardized title. In headless mode the messages will be shown on terminal instead. (Also added myself to contributors list)

No html output to console.

41999f6

Now translating html text to plain text before sending it to console.

Added ShowXxxWait messageboxes.

ad3194c

ShowXxxWait messageboxes will wait for the/a button to be pressed in gui mode. ShowXxxWait messageboxes also have an optional 'abort' button and a default selection. In HEADLESS mode there is NO wait, and the default selection is returned.

Use ShowErrorWait in main

ea5553a

Also added catching of generic exceptions withusing a ShowErrorWait

Fixing HEADLESS build...

28a042b

Can't use QTextEdit in messages Unused variables in main

Merge branch 'global-messages' of github.com:pgScorpio/jamulus into g…

a6f8843

…lobal-messages

pljones moved this from Waiting on Team to Triage in Tracking (old) Oct 5, 2022

ann0see changed the base branch from master to main February 12, 2023 19:08

ann0see closed this Jul 1, 2023

Tracking (old) automation moved this from Triage to Done Jul 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of global messages #2598

Implementation of global messages #2598

pgScorpio commented Apr 14, 2022 •

edited by ann0see

Loading

ann0see commented Apr 15, 2022

ann0see commented Apr 15, 2022 •

edited

Loading

ann0see left a comment •

edited

Loading

ann0see Apr 15, 2022

pgScorpio Apr 15, 2022 •

edited

Loading

pgScorpio Apr 15, 2022

ann0see Apr 16, 2022

ann0see Apr 16, 2022

pgScorpio Apr 17, 2022

pgScorpio commented Apr 15, 2022

pgScorpio commented Apr 15, 2022 •

edited

Loading

hoffie left a comment

ann0see Apr 16, 2022

pgScorpio Apr 17, 2022

ann0see Apr 30, 2022

ann0see Apr 16, 2022

pgScorpio Apr 17, 2022

ann0see commented Apr 16, 2022 •

edited

Loading

ann0see commented Apr 16, 2022 •

edited

Loading

pgScorpio commented Apr 16, 2022 •

edited

Loading

pgScorpio commented Apr 16, 2022 •

edited

Loading

ann0see commented Jul 1, 2023

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

Implementation of global messages #2598

Implementation of global messages #2598

Conversation

pgScorpio commented Apr 14, 2022 • edited by ann0see Loading

Checklist

ann0see commented Apr 15, 2022

ann0see commented Apr 15, 2022 • edited Loading

ann0see left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio commented Apr 15, 2022

pgScorpio commented Apr 15, 2022 • edited Loading

hoffie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Apr 16, 2022 • edited Loading

ann0see commented Apr 16, 2022 • edited Loading

pgScorpio commented Apr 16, 2022 • edited Loading

pgScorpio commented Apr 16, 2022 • edited Loading

ann0see commented Jul 1, 2023

pgScorpio commented Apr 14, 2022 •

edited by ann0see

Loading

ann0see commented Apr 15, 2022 •

edited

Loading

ann0see left a comment •

edited

Loading

pgScorpio Apr 15, 2022 •

edited

Loading

pgScorpio commented Apr 15, 2022 •

edited

Loading

ann0see commented Apr 16, 2022 •

edited

Loading

ann0see commented Apr 16, 2022 •

edited

Loading

pgScorpio commented Apr 16, 2022 •

edited

Loading

pgScorpio commented Apr 16, 2022 •

edited

Loading