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

FEAT(client): Log a message if some change could not be saved due to missing certificate #4301

Merged
merged 2 commits into from Jun 20, 2020
Merged

FEAT(client): Log a message if some change could not be saved due to missing certificate #4301

merged 2 commits into from Jun 20, 2020

Conversation

Popkornium18
Copy link
Contributor

@Popkornium18 Popkornium18 commented Jun 17, 2020

Log a message if a change like local muting of a user could not be saved because this user does not have a certificate.

The displayed message is:

This change could not be saved permanently and is lost on restart because Username does not have a certificate.

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

I think the logging of the error should be done via a function inside MainWindow in order to reduce code-duplication.

In order for the translation to work I'd suggest something like

void MainWindow::logChangeNotPermanent(QString &actionName) const {
    msgBox(WObject::tr("%1 could not be saved permanently and is lost on restart because %2 does not have a certificate.").arg(actionName).arg(Log::formatClientUser(p, Log::Target)));
}

And then translating the action strings separately when calling this function.

An additional point is also the use of msgBox. Afaik we normally don't use that for logging (non-critical) errors. Instead we use the log object to log these errors to the console...

Furthermore it'd be nice if you could use braces for all if statements you added. I know lots of code doesn't use it but we're aiming at improving the code base and that'll only work gradually :)

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/UserLocalVolumeDialog.cpp Outdated Show resolved Hide resolved
@Popkornium18
Copy link
Contributor Author

An additional point is also the use of msgBox. Afaik we normally don't use that for logging (non-critical) errors. Instead we use the log object to log these errors to the console...

I assume you refer to the member of class LogMessage and not console as in stdout?

@Krzmbrzl
Copy link
Member

I assume you refer to the member of class LogMessage and not console as in stdout?

yes

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.h Show resolved Hide resolved
@Popkornium18
Copy link
Contributor Author

I have added the changes you requested

Log a message if a change like local muting of a user
could not be saved because this user does not have a certificate.
Updating 'mumble_en.ts'...
    Found 1880 source text(s) (5 new and 1875 already existing)
@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature labels Jun 20, 2020
@Krzmbrzl Krzmbrzl added this to the 1.4.0 milestone Jun 20, 2020
@Krzmbrzl Krzmbrzl merged commit a20d167 into mumble-voip:master Jun 20, 2020
@Krzmbrzl
Copy link
Member

Thank your for your contribution! :D

@Popkornium18 Popkornium18 deleted the settings_info_no_certificate branch June 20, 2020 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants