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

Add command-line flags for accessing license information for Mumble and Murmur #2644

Merged
merged 8 commits into from Nov 25, 2016

Conversation

@mkrautz
Copy link
Member

commented Nov 16, 2016

We discussed this long time ago. Thorvald wanted Murmur to have license info embedded into the binary.

Please follow along with the commit messages to see the story unfold. :-)

@mkrautz mkrautz changed the title Add command-line flags for accessing license information for Mumble and Murmur WIP: Add command-line flags for accessing license information for Mumble and Murmur Nov 16, 2016

@mkrautz mkrautz force-pushed the mkrautz:license branch 2 times, most recently from 85f2eae to a11f32e Nov 20, 2016

@mkrautz mkrautz changed the title WIP: Add command-line flags for accessing license information for Mumble and Murmur Add command-line flags for accessing license information for Mumble and Murmur Nov 20, 2016

@mkrautz mkrautz force-pushed the mkrautz:license branch 3 times, most recently from 7debdc7 to 5e17e20 Nov 20, 2016

QString License::printableThirdPartyLicenseInfo() {
QString output;

QList<LicenseInfo> thirdPartyLicenses = License::thirdPartyLicenses();

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Why the License:: qualifier?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

I think it makes it more apparent that it's a static method.


QList<LicenseInfo> thirdPartyLicenses = License::thirdPartyLicenses();
foreach (LicenseInfo li, thirdPartyLicenses) {
QString header = QString::fromLatin1("%1 (%2)").arg(li.name).arg(li.url);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Personally, I think I would prefer the newlines for both header and headerHorizLine to be added in them, rather than added after-the-fact below.
Was this a deliberate choice to have the individual newlines visible below?
I think only having to add newlines for "empty lines" below would be better. The strings declared here always have a newline after them.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Makes sense...

}

return output;
}

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

No newline at end of file it says

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Will fix.

@Kissaki
Copy link
Member

left a comment

Is the About Dialog (almost) the same as in Mumble?

QTabWidget *qtwTab = new QTabWidget(this);
QVBoxLayout *vblMain = new QVBoxLayout(this);

QTextEdit *qteLicense=new QTextEdit(qtwTab);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Missing spacing around assignment operator.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Copied from src/mumble/About.cpp, but will fix.


qtb3rdPartyLicense->moveCursor(QTextCursor::Start);

QWidget *about=new QWidget(qtwTab);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Spacing

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Copied from src/mumble/About.cpp, but will fix.


QWidget *about=new QWidget(qtwTab);

QLabel *icon=new QLabel(about);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Spacing

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Copied from src/mumble/About.cpp, but will fix.

QIcon windowIcon = QApplication::windowIcon();
icon->setPixmap(windowIcon.pixmap(windowIcon.actualSize(QSize(128, 128))));

QLabel *text=new QLabel(about);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Spacing

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Copied from src/mumble/About.cpp, but will fix.

).arg(QLatin1String(MUMBLE_RELEASE))
.arg(QLatin1String("http://www.mumble.info/"))
.arg(QLatin1String("Copyright 2005-2016 The Mumble Developers")));
QHBoxLayout *qhbl=new QHBoxLayout(about);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Spacing

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

Copied from src/mumble/About.cpp, but will fix.

@@ -14,12 +14,21 @@
# include <QtGui/QDialog>
#endif

enum AboutDialogOptions {

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

Rather then iterating, why did you bit-flag them? For potential sub-tabbing and specifying two at the same time?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 23, 2016

Author Member

My thinking was that we might add other flags in the future.
Right now, these flags are mutually exclusive, so == works for now, as done in About.cpp. (Maybe I should document that?)

...Maybe I'm just overthinking things, and we should just let the compiler number these?
It's not a public API anyway, so if we were to do any changes to it, we'd rip it apart anyway...

Dropping it.

@Kissaki
Copy link
Member

left a comment

Commit message:

This doesn't work for -license, -authors and -third-party-licenses.

Maybe add why not, like in the last commit? Because its too much text.

#ifdef Q_OS_WIN
AboutDialog ad(NULL, AboutDialogOptionsShowLicense);
ad.exec();
return 0;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Nov 22, 2016

Member

This behaviour is different to -version then. I don’t see calls to ShowMessageDialog here as the commit message claims.

I windows server administration never headless? What happens if it is? Maybe we should still do the qFatal for logging the info, in case the dialog can not be opened?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 22, 2016

Author Member

On Windows, Murmur calls ShowMessageDialog() for qFatal. It's defined at

mumble/src/murmur/main.cpp

Lines 115 to 128 in 54eab8d

if (type == QtFatalMsg) {
#ifdef Q_OS_UNIX
if (detach) {
if (qlErrors.isEmpty())
qlErrors << msg;
qlErrors << QString();
m = qlErrors.join(QLatin1String("\n"));
fprintf(stderr, "%s", qPrintable(m));
}
#else
::MessageBoxA(NULL, qPrintable(m), "Murmur", MB_OK | MB_ICONWARNING);
#endif
exit(1);
}

This is because Murmur on Windows is built a a GUI app with a GUI log, a systray icon, etc. -- we can't easily log to stdout/stderr, since there's no console.

So, I'm not really doing this just because it's nicer to have a gui on Windows. I'm doing it because Murmur on Windows is a GUI app, and implementing -license, -authors and -third-party-licenses using qFatal() causes a message dialog that's too large to fit on anyone's screen to show up.

I felt the best solution was to add an About dialog. Users running Murmur on Windows can now see version/license information while running it, and the CLI flags are now usable (you can scroll through the license texts).

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2016

@Kissaki wrote:

Commit message:

This doesn't work for -license, -authors and -third-party-licenses.

Maybe add why not, like in the last commit? Because its too much text.

Sure. I think that commit message warrants a re-write. It could also explain where the ShowMessageDialog() happens.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2016

Thanks for the review. I will try to address the comments tomorrow.

@mkrautz mkrautz force-pushed the mkrautz:license branch from 5e17e20 to af8fad4 Nov 23, 2016

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2016

@Kissaki PTAL

@Kissaki

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Reviewing a second iteration is cumbersome with this interface.
Should be fine.
You did not fix the missing operator spacing on the copied code? You did not answer my second last inline question (about the enum values)?

The second-last commit uses the dialog constructor parameter introduced in the last commit, so the second-last commit is not valid code?

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2016

GitHub rearranged the commits. The way they're shown here is not actually their order.
It's sorted by time, apparently... This SUCKS.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2016

Hmm, operator spacing ... that'd odd. I squash that into the existing about dialog change. Hm.

@mkrautz mkrautz force-pushed the mkrautz:license branch from af8fad4 to 1075c2f Nov 23, 2016

mkrautz added 8 commits Nov 23, 2016
Move licenses.h header to src/ from src/mumble/.
We'll need it to display license information in
Murmur.
License: new class for easier access to license data.
This new class wraps the existing "licenses.h"-header,
but makes it easier to use.

The existing "licenses.h"-header uses static string
literals, which means that if two source files include
the file, both object files will include the string data.

This License file is introduced to avoid that problem,
but also to provide a more Qt-like API, with QStrings
instead of C-style char *s.
mumble: implement --license, --authors, and --third-party-licenses fl…
…ags.

This commit adds command-line flags for Mumble to acquire license
information.

The --license flag shows the LICENSE file.

The --authors flag shows the AUTHORS file.

The --third-party-licenses shows license information
for third-party licenses.

We also add -license, -authors and -third-party-licenses flags
for consistency with Murmur. Mumble tends to use --gnu-style-parameters,
while Murmur tends to use -single-dash-parameters.
For these new parameters, we might as well make both forms work for
both programs.
murmur: add -license, -authors and -third-party-licenses flags.
We also add aliases --license, --authors and --third-party-licenses,
just like Mumble has.
Murmur: Add AboutDialogOptions flag to the AboutDialog class.
This flag allows users of AboutDialog to determine which
tab of the about dialog (About, License, Authors,
Third-party Licenses) should be active when the dialog
is first shown.

This is needed for the license command-line flags on Windows,
where qFatal() output is shown via Windows's ShowMessageDialog.
Unfortunately, both the AUTHORS file and the combined
third-party license text are too big to display in a regular
dialog.

To avoid this, we'll show the AboutDialog when --license,
--authors and --third-party-licenses are passed to Murmur
on Windows.

This commit only implements the AboutDialog-related changes
necessary to implement this.
Murmur: use About dialog for -license, -authors and -third-party-lice…
…nses on Windows.

On Windows, Murmur uses ShowMessageDialog for qFatal() output.

This doesn't work for -license, -authors and -third-party-licenses,
because the AUTHORS and third-party license texts are too big to
show in a regular dialog

To fix this, we instead pop up Murmur's AboutDialog when
any of the license-related command-line flags are passed
to Murmur.

@mkrautz mkrautz force-pushed the mkrautz:license branch from 1075c2f to 25cb53d Nov 23, 2016

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2016

I've fixed the operator spacing, and removed the bitflags from AboutDialogOptions.

@mkrautz mkrautz merged commit 82c27fe into mumble-voip:master Nov 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.