Rewrite servicemenu helper utility in C++
ClosedPublic

Authored by aspotashev on Jun 18 2019, 1:00 AM.

Details

Summary
  • Also support MIME type "application/x-compressed-tar".
  • Update tests in Ruby, remove SimpleCov.

BUG: 399229

Test Plan

Ruby tests passed

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aspotashev created this revision.Jun 18 2019, 1:00 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptJun 18 2019, 1:00 AM
aspotashev requested review of this revision.Jun 18 2019, 1:00 AM
aspotashev updated this revision to Diff 60008.Jun 18 2019, 1:01 AM

update commit message

aspotashev edited the summary of this revision. (Show Details)Jun 18 2019, 1:02 AM
cfeck added a subscriber: cfeck.Wed, Jul 3, 3:15 PM

Any objection to get this into 19.08?

elvisangelaccio added inline comments.Wed, Jul 3, 7:56 PM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
36–43

Since we are in a Qt program now, we could just open a KMessageBox rather than spawn another process just to show an error dialog (and kdialog might not even be installed).

72–78

Same here. We should use QMimeDatabase rather than run xdg-mime.

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
35–42
Since we are in a Qt program now, we could just open a KMessageBox rather than spawn another process just to show an error dialog (and kdialog might not even be installed).

71–77
Same here. We should use QMimeDatabase rather than run xdg-mime.

This patch is about rewrite to C++. What you suggest are good ideas, but can be done as separate patches, from my POV they are of lower priority.

Besides the two issues you mentioned, the current implementation also has a huge bug that stops many services menus from being installed: the installation scripts are run from a wrong current working dir. I already have a patch to fix it, however I will post it when this one lands because otherwise it's hard to publish the second patch with Phabricator.

To be exact, KMessageBox is not equivalent to kdialog --passivepopup.

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
35–42
Since we are in a Qt program now, we could just open a KMessageBox rather than spawn another process just to show an error dialog (and kdialog might not even be installed).

71–77
Same here. We should use QMimeDatabase rather than run xdg-mime.

This patch is about rewrite to C++. What you suggest are good ideas, but can be done as separate patches, from my POV they are of lower priority.

Besides the two issues you mentioned, the current implementation also has a huge bug that stops many services menus from being installed: the installation scripts are run from a wrong current working dir. I already have a patch to fix it, however I will post it when this one lands because otherwise it's hard to publish the second patch with Phabricator.

To be exact, KMessageBox is not equivalent to kdialog --passivepopup.

Fair enough.

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
32

How about Q_NORETURN ?

aspotashev updated this revision to Diff 61298.Sun, Jul 7, 9:33 PM
aspotashev edited the summary of this revision. (Show Details)
  • servicemenuinstaller: Use Q_NORETURN
aspotashev marked an inline comment as done.Sun, Jul 7, 9:34 PM

Thanks. Didn't know Q_NORETURN existed.

elvisangelaccio requested changes to this revision.Sun, Jul 14, 8:32 PM
elvisangelaccio added inline comments.
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
255

This will detach the container, please use qAsConst

269

This will detach the container, please use qAsConst

303

This will detach the container, please use qAsConst

320–321

This will detach the container, please use qAsConst

This revision now requires changes to proceed.Sun, Jul 14, 8:32 PM
aspotashev updated this revision to Diff 61758.Sun, Jul 14, 8:54 PM
  • servicemenuinstaller: Use qAsConst
elvisangelaccio accepted this revision.Sun, Jul 14, 9:12 PM
This revision is now accepted and ready to land.Sun, Jul 14, 9:12 PM
aspotashev updated this revision to Diff 61759.Sun, Jul 14, 9:15 PM
  • servicemenuinstaller: Fix build
This revision was automatically updated to reflect the committed changes.
aspotashev added inline comments.Sun, Jul 14, 11:39 PM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
36–43

I checked the sources of kdialog and discovered that what "kdialog --passivepopup" does is not trivial, see https://cgit.kde.org/kdialog.git/tree/src/kdialog.cpp#n517

Firstly it tries to show a Plasma/shell notification over D-Bus. If unavailable, it falls back to KPassivePopup.

In the Beautiful KNewStuff of the Future, we could pass the error message back to the "Get New Stuff" dialog and display it there as an inline notification. One of the ways to implement this would be a plugin system where Dolphin's servicemenuinstaller is a plugin (.so/.dll).

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
36–43

I see, thanks for investigating it.