This is the context menu plugin - the first target in CMakeLists.txt - which was never ported to KF5. This change implements it using the new KAbstractFileItemActionPlugin plugin system.
Details
Details
- Reviewers
dfaure - Group Reviewers
Konqueror Plasma - Commits
- R226:0fa3ccc7fbf6: Akregator feed detector plugin: Restore the popup menu action
Built Konqueror with this change. Checked that the context menu over an RSS link (e.g. as on dot.kde.org) has an action item "Add Feed to Akregator", and that selecting this action sends the feed URL to Akregator.
Diff Detail
Diff Detail
- Repository
- R226 Konqueror
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Nice
| plugins/akregator/akregatorplugin.cpp | ||
|---|---|---|
| 82 | coding style: "return acts", no parenthesis needed | |
| 95 | This reads like it would trigger false positives easily. I wonder if this code meant to say endsWith... | |
| plugins/akregator/akregatorplugin.h | ||
| 40 | override ? | |
| 44–45 | while at it: const, or static, or better, make it file-static | |
| 44–45 | white at it: private | |
| 45–46 | const | |
Comment Actions
Diff updated in accordance with review comments: coding style, modern C++ features and reducing visibility wherever possible.
David, would you also be able to look at https://phabricator.kde.org/D12616 - KF5 conversion of the other target - so that they can both be committed in sequence?
| plugins/akregator/akregatorplugin.h | ||
|---|---|---|
| 44–45 | Made file-static, but left the next function as member to avoid having to construct m_feedMimeTypes every time (or complicate with global static). | |
| plugins/akregator/akregatorplugin.cpp | ||
|---|---|---|
| 95 | Not sure what that test was intended to catch either, but as you say looking for "xml" anywhere in the path will accept many links that aren't feeds. Rewrote this test to only look at the URL path (not the full URL, including scheme/host/query!), and only accept "rss" or "rdf" within that path. This string test is only done if the MIME type test fails, anyway, so hopefully for a properly configured web site the MIME type test will accept the link. | |
Comment Actions
Still strange that "rss" or "rdf" anywhere in the path is enough, but I'm no rss expert... Looks good overall.
| plugins/akregator/akregatorplugin.cpp | ||
|---|---|---|
| 72 | BTW why the '{' on separate lines? The Qt5/KF5 coding style is to have it on the same line, and you left it that way further down in this file, which is now inconsistent with itself. | |
| plugins/akregator/akregatorplugin.h | ||
| 44–45 | Sure, the other function should stay. | |
| plugins/akregator/akregatorplugin.cpp | ||
|---|---|---|
| 72 | Will fix in commit. | |