FEAT(client): Adding Save Image Functionality to View Comment (other users) and change comment (self)

[sunidhimaja]

Implements #1935

Checks

• My commits follow the commit guidelines

[sunidhimaja]

Where would make sense for adding a test for this feature (if at all)? @davidebeatrici @Hartmnt ... I have ensured it works locally but am curious if I can do anything more!

[Hartmnt]

Where would make sense for adding a test for this feature (if at all)?

There are only very few unit tests in Mumble. No need to do anything on that front.

I have ensured it works locally but am curious if I can do anything more!

We need some time to review your change ☺️

For the time being, you can squash both commits into one. No need to have a separate commit for the formatting changes.
Yes, this will require you to force-push your branch, but that is fine for us. Look up some online tutorial, if you don't feel comfortable enough with git.

[Hartmnt]

I have tested your changes. When opening the "Save as" file dialog, there is this checkbox:

When this is checked, saving the image will not work. I assume the reason is that you encoded the file filters with a , which does not seem to be how Qt wants this handled. When I uncheck the box, it seems to work. Please take a closer look at that. This will need to be fixed.

Other than that, please consider:

[Hartmnt]

Nice work, removing the , seems to have fixed the remaining usability issues. So this is now working as intended.

I still have some things for you to tackle:

1. Your commits are not associated with a GitHub account. This is probably because you used a different Email here on GitHub than in your git config user.email. We can still merge this without your commits being associated with your GitHub account, but then your GitHub Account will not be listed as a contributor to Mumble.
   If you want to fix this, you need to make sure your git config user.email is the same as your GitHub Email address and re-author your commits.

2. Please make sure that your changes are within one commit. To do that you could squash the existing commits into one. If you don't know how to do that, lookup some online tutorials. You probably want to use something like git rebase -i or something similar.

3. When everything works and there is only one commit in this MR, we can fix the translation CI pipeline with out updateTranslations script

4. Also some code refactors:

[Hartmnt]

This is now unused

[Hartmnt]

getRichTextEdit is now unused

[Hartmnt]

getRichTextEdit is now unused

[Hartmnt]

{ and } for single line ifs

Suggested change

	if (!fileName.isEmpty()) {
	if (QFileInfo(fileName).suffix().isEmpty())
	fileName += ".png"; // default to PNG if no extension
	image.save(fileName);
	}
	if (!fileName.isEmpty()) {
	if (QFileInfo(fileName).suffix().isEmpty()) {
	fileName += ".png"; // default to PNG if no extension
	}
	image.save(fileName);
	}

[Hartmnt]

Don't know what your thinking was behind this, but I would rather still have this inline with the action creation

Suggested change

	menu->addAction(saveText, [=]() {
	menu->addAction(tr("Save Image As…"), [=]() {

[Hartmnt]

Don't know what your thinking was behind this, but I would rather still have this inline with the action creation

[Hartmnt]

I think it would be nice, if you move the setContextMenuPolicy initialization bit into the constructor.

Then move the connect to the constructor as well, referencing your new method. Something like this:

connect(qteRichText, &QTextEdit::customContextMenuRequested, this, addContextMenuToSaveImages)

and rename addContextMenuToSaveImages to something like addCustomContextMenuActions. That way we can add more actions in this method in the future.