This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123342/

So firstly, including a dependency on knewstuff just to get at the KMoreTools is imho not acceptable. KMoreTools does not belong in KNewStuff. That is the entire point of frameworks: modularity, ability to choose based on specific functionality.

Otherwise, there are a few comments in the code below.

ksnapshotsendtoactions.cpp (Diff revision 2)
		50	/*
		51	* Some hints for compiling with KIPI
		52	* ----------------------------------
		53	* Kipi must be installed:
		54	* e.g. $ ./kdesrc-build libkipi libkexiv2 libkdcraw kipi-plugins
		55	*
		56	* In any case, make sure to do cmake again in order to have the variable
		57	* KIPI_FOUND set correctly.
		58	*
		59	* Maybe you need to call
		60	* $ kbuildsycoca5 --noincremental
		61	* to update the kservice registry.
		62	*/

This does not belong in the code. It will eventually be outdated and comments in code like this bitrot quickly. Place it in the README perhaps.

ksnapshotsendtoactions.cpp (Diff revision 2)
		129	QMenu* otherAppsHostMenu = moreMenu;
		130	if (!otherAppsHostMenu) {
		131	otherAppsHostMenu = menu;
		132	}

why not just
if (!moreMenu) {
     moreMenu = menu;
}
moreMenu->addSeparator();
[..]
?

ksnapshotsendtoactions.cpp (Diff revision 2)
QList<QAction*> KSnapshotSendToActions::createSendToActions()
	200	// TODO (in kipi?):
	201	// The order of the actions of one kipi plugin (e.g. print images)
	202	// is not stable (the order of "Print images" and "Print assistant..."
	203	// may change between ksnapshot sessions).
	204	// So for now we use the action's text() which works*) as long
	205	// as the user does not switch the language.
	206	// *) for correctly restore user settings of KMoreTools

you can't rely on ordering anyways, as the user may install new plugins (or remove existing ones) between launches. so you need something identifiable. Does kipi happen to add anything useful to the QAction* like a QVariant in data(), or perhaps even a QObject::objectName()?

That would at least be more stable...

- Aaron J. Seigo

On April 15th, 2015, 9:15 p.m. UTC, Gregor Mi wrote:

Review request for KDE Graphics, KSnapshot and Aaron J. Seigo.

By Gregor Mi.

Updated April 15, 2015, 9:15 p.m.

Bugs: 274928, 287663, 319901

Repository: ksnapshot

Description

Currently, ksnapshot's "Send To..." menu is one long list of items which can be quite large depending on the number of services and kipi-plugins installed. Note, that in the provided screenshot, the list is not that long because not all kipi plugins are ported yet. In my daily screenshot work I always use the same set of tools and therefore it would be nice if the menu was user-configurable.

SUGGESTION: - Only show up to 10 items in the main menu and move the rest to a new "More" submenu. - Implement with KMoreTools (see https://git.reviewboard.kde.org/r/122910/) - Since KMoreTools is used, the menu is automatically user configurable. See screenshots.

Diffs

ksnapshotsendtoactions.cpp (a8c4ccbb72cee3bef486af417aa637c3f41de48d)
ksnapshotsendtoactions.h (f0b4f8f6ccea15b169d7ef149645e85d89b5fe37)
ksnapshot.cpp (ac0f2c4f44d47e1a4ae4a318382253a23ad1ed4a)
CMakeLists.txt (cdab928a7db0e7ea29fba35880250639461dbb1e)
CMakeLists.Sources.txt (522fc3f0f943e5c2856c76e93174d39661ca9c50)

View Diff

File Attachments

current situation - SendTo menu

Reduced SendTo menu after custom configuration

remaining items in "More" submenu

Configure Menu dialog

Configure Menu dialog after Reset was clicked (first 10 items in "Main section" by default)