D8652: Add supported transformations to OutputDevice
sebas added a comment. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. Dingdong? REPOSITORY R127 KWayland BRANCH supported-transformations REVISION DETAIL https://phabricator.kde.org/D8652 To: graesslin, #frameworks, #kwin, #plasma, sebas, davidedmundson Cc: sebas, davidedmundson, plasma-devel, schernikov, leezu, ZrenBot, ngraham, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, apol, mart, hein
D9335: invalidate the runtime cache on install
sebas accepted this revision. This revision is now accepted and ready to land. REPOSITORY R290 KPackage BRANCH packagefix REVISION DETAIL https://phabricator.kde.org/D9335 To: mart, #plasma, sebas Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9108: Remove implicit string casting
sebas accepted this revision. This revision is now accepted and ready to land. REPOSITORY R242 Plasma Framework (Library) BRANCH master REVISION DETAIL https://phabricator.kde.org/D9108 To: apol, #plasma, #frameworks, sebas Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8652: Add supported transformations to OutputDevice
sebas accepted this revision. sebas added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > outputdevice.h:244 > + * @returns The transformations supported on this OutputDevice > + * @since 5.XX > + * @see supportedTransformationAdded 5.40 I guess :) > outputdevice_interface.h:133 > + * Due to that there is no remove operation for supported > transformations. > + * @since 5.XX > + **/ 5.40 REPOSITORY R127 KWayland BRANCH supported-transformations REVISION DETAIL https://phabricator.kde.org/D8652 To: graesslin, #frameworks, #kwin, #plasma, sebas Cc: sebas, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart
D7898: Only send OutputConfig sendApplied / sendFailed to the right resource
sebas added a comment. FWIW, I tested it, it fixes kwin crashing when adding a setApplied() call on the OutputConfiguration, and it also means the applied signal arrives. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D7898 To: davidedmundson, #plasma, sebas Cc: sebas, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, apol, mart, hein
D7898: Only send OutputConfig sendApplied / sendFailed to the right resource
sebas accepted this revision. sebas added a comment. This revision is now accepted and ready to land. Yes, the initial idea was to send applied() to all connected resources. That is not s_allResources, so your patch is correct. The resources that are actually bound are not currently tracked. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D7898 To: davidedmundson, #plasma, sebas Cc: sebas, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, apol, mart, hein
Re: kaboutlicense api extension ::spdxId()?
On dinsdag 11 juli 2017 13:41:17 CEST Harald Sitter wrote:
> I was wondering if anyone had an opinion on extending kaboutlicense
> with a ::spdxId() instance method returning the license's spdx id [1].
>
> Use case at hand is kpackagetool, which maps X-KDE-PluginInfo-License
> of (e.g.) plasma applets to appstream metadata. Appstream however uses
> the standardized spdx identifiers so we need a conversion table
> somewhere. And here I am thinking why not kaboutlicense itself.
>
> QString KAboutLicense::spdxId() const
> {
> switch (d->_licenseKey) {
> case KAboutLicense::File:
> return QStringLiteral("");
> case KAboutLicense::GPL_V2:
> return QStringLiteral("GPL-2.0");
>...
> }
>
> [1] https://spdx.org/licenses/
This would make sense and allow 3rd party tools using SPDX keys to make sense
of X-KDE-PluginInfo-License keys, indeed. I think it's useful.
> (on a side node: it may be useful to switch our plugininfos to spdx
> ids by default at some point in the future, our current format is not
> only deficient in the lack of standardization but also doesn't cover
> 'and later' qualification at all)
It does parse "and later", it's indicated by the + sign, but it's not
reflected in the enum, GPLv2+ would be mapped to GPLv2, so you're right, our
current system is lacking in that regard (but could be extended, although I
don't know what would break then.
Cheers,
--
sebas
http://www.kde.org | http://vizZzion.org
Re: KTitleWidget and the native Mac style
On dinsdag 4 juli 2017 17:55:01 CEST René J.V. Bertin wrote: > Me neither for myself, I'm happy with how my QtCurve theme looks but I'm > also trying to improve the way KDE software looks using the native style. The frame in my understanding is old weight, and can go (do check with the VDG though!), but the larger font isn't. It's semantically a sane choice (titles often have larger fonts), but to me, more importantly, reducing its size will lead to regressions in Plasma. I have no problems with moving its rendering to the style so it can depend on the platform used, or another technically sound solution, but a change should not introduce regressions in Plasma. Cheers, -- sebas http://www.kde.org | http://vizZzion.org
Re: Kirigami in Frameworks
On zaterdag 1 juli 2017 11:27:21 CEST Albert Astals Cid wrote: > https://websvn.kde.org/trunk/l10n-kf5/templates/messages/frameworks/libkirig > ami2plugin_qt.pot?revision=1492189=markup > > Disagrees with you Ow, okay. I see, I only grepped for i18n, not for qsTr(), sorry for the noise! > On ds., jul. 1, 2017 at 9:02, Sebastian Kügler > <se...@kde.org> wrote: > > On vrijdag 30 juni 2017 23:16:49 CEST Albert Astals Cid wrote: > > What happened to the "no new strings after the first two weeks" rule? > > Kirigami doesn't have any translatable strings. -- sebas http://www.kde.org | http://vizZzion.org
Re: Kirigami in Frameworks
On vrijdag 30 juni 2017 23:16:49 CEST Albert Astals Cid wrote: > What happened to the "no new strings after the first two weeks" rule? Kirigami doesn't have any translatable strings. -- sebas http://www.kde.org | http://vizZzion.org
Re: unwanted breeze('ish) icons in KFileWidget on Mac
On dinsdag 20 juni 2017 16:10:59 CEST René J.V. Bertin wrote: > So where do those icons come from, and why are some applications immune? > Pointers highly appreciated! A screenshot says more than a thousand words... :) -- sebas http://www.kde.org | http://vizZzion.org
D6215: Introduce aboutToShow() signal
sebas added a comment. I like this, and I'll likely need it in my kscreen OSD code aswell (positioning there is unreliable, and my best guess is that this is the exact same problem @mart is trying to solve here). +1 REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D6215 To: mart, #plasma, davidedmundson Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas
D5747: add pid to plasma window management protocol
sebas closed this revision. sebas added a comment. Issue has been fixed elsewhere, otherwise code is submitted. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5747 To: sebas, #plasma, hein, graesslin Cc: apol, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, mart, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas abandoned this revision. sebas added a comment. Okay, thanks David. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: graesslin, bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas added a comment. Possibly, in this case, I disagree. The pid is initially unknown set to in the client, and can change afterwards. In reality, the pid of the window doesn't change, but it can still be set after being initially 0. In fact, it has to be. In the end, I don't care much, but having it connected to dataChanged makes this whole thing way more consistent, and it makes sense semantically. IMO, not putting it behind dataChanged is semantic wanking making the interface harder to understand, not easier, and not better. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas requested review of this revision. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: bshah, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas added a comment. What's d581? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5872 To: sebas, #plasma, hein, davidedmundson Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5872: pidChanged also signals dataChanged in WindowModel
sebas created this revision. Restricted Application added projects: Plasma on Wayland, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY Eike removed this, but I'm not sure why. All other roles trigger datachanged, so I don't see why pid shouldn't. This patch fixes the tests, which I should not have broken in the first place. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D5872 AFFECTED FILES src/client/plasmawindowmodel.cpp To: sebas, #plasma, hein Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5747: add pid to plasma window management protocol
This revision was automatically updated to reflect the committed changes. Closed by commit R127:0b4d8a7fc1df: add pid to plasma window management protocol (authored by sebas). REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5747?vs=14442=14463 REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp autotests/client/test_wayland_windowmanagement.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: apol, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, hein, lukas
D5747: add pid to plasma window management protocol
sebas updated this revision to Diff 14442. sebas added a comment. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. - Update docs: the pid is just set, but doesn't logically change - ws-- REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5747?vs=14418=14442 BRANCH sebas/processid REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp autotests/client/test_wayland_windowmanagement.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: apol, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, hein, lukas
D5747: add pid to plasma window management protocol
sebas updated this revision to Diff 14418. sebas added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. - Drop pid change handling in the model code - Extend model test --Eike on sebas' box REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5747?vs=14410=14418 BRANCH sebas/processid REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp autotests/client/test_wayland_windowmanagement.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D5747: add pid to plasma window management protocol
sebas updated this revision to Diff 14410. sebas added a comment. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. - also test default REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5747?vs=14407=14410 BRANCH sebas/processid REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp autotests/client/test_wayland_windowmanagement.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5747: add pid to plasma window management protocol
sebas updated this revision to Diff 14407. sebas marked 3 inline comments as done. sebas added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. Address review comments - Minor corrections in docs - Use uint for the pid - uint32 for pids instead of int32 - Fix @since in docs - send initial value when it's not 0 - Add autotest in TestWindowManagement as well REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5747?vs=14249=14407 BRANCH sebas/processid REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp autotests/client/test_wayland_windowmanagement.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D5812: Add missing block to make sure initial pid is sent
sebas added a comment. Adding it to https://phabricator.kde.org/D5747 for merge simplicity. @hein please abandon. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D5812 To: hein, #plasma, sebas, graesslin Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
D5747: add pid to plasma window management protocol
sebas created this revision. Restricted Application added projects: Plasma on Wayland, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY This patch adds a pid event to the plasma window management protocol. It allows the compositor to tell allow a mapping between windows and processes. Bumps the version number of the interface to 8 to indicate this. TEST PLAN autotest added, passed REPOSITORY R127 KWayland BRANCH sebas/processid REVISION DETAIL https://phabricator.kde.org/D5747 AFFECTED FILES autotests/client/test_plasma_window_model.cpp src/client/plasmawindowmanagement.cpp src/client/plasmawindowmanagement.h src/client/plasmawindowmodel.cpp src/client/plasmawindowmodel.h src/client/protocols/plasma-window-management.xml src/server/plasmawindowmanagement_interface.cpp src/server/plasmawindowmanagement_interface.h To: sebas, #plasma, hein, graesslin Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, lukas
D5721: reload icon when usesPlasmaTheme changes
sebas accepted this revision. This revision is now accepted and ready to land. REPOSITORY R242 Plasma Framework (Library) BRANCH phab/useplasmatheme REVISION DETAIL https://phabricator.kde.org/D5721 To: mart, #plasma, sebas Cc: plasma-devel, #frameworks, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
[Differential] [Accepted] D4689: IconItem: Add roundToIconSize property
sebas accepted this revision.
sebas added a comment.
This revision is now accepted and ready to land.
Almost good, you can add the signalspy and then ship it from my side.
INLINE COMMENTS
> iconitemtest.cpp:526
> +
> +item->setProperty("roundToIconSize", false);
> +
Might as well check for the roundToIconSizeChanged signal here as well. We
should test what we reasonably can, and that's an easy one.
> drosca wrote in iconitem.h:147
> The property is documented, I think there's no point in documenting the
> getters/setters as you can't use them from QML anyway.
Right. :)
REPOSITORY
R242 Plasma Framework (Library)
BRANCH
arcpatch-D4689
REVISION DETAIL
https://phabricator.kde.org/D4689
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: drosca, sebas, #plasma
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff,
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol
[Differential] [Commented On] D4831: Add new component for the greyed out labels in Item Delegates.
sebas added a comment. I like it. If kbroulik is happy, consider that a +1 from me, too. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D4831 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: davidedmundson, #plasma Cc: sebas, broulik, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol
[Differential] [Requested Changes] D4689: IconItem: Add roundToIconSize property
sebas requested changes to this revision.
sebas added a comment.
This revision now requires changes to proceed.
I think a problem with using roundToIconSize as isolated property is that it
really isn't. It has intended effects on the sizing of the item, but the
current version of the patch doesn't reflect that. I think it needs to trigger
a series of size change signals. See my comments inline.
INLINE COMMENTS
> iconitemtest.cpp:524
> +
> +item->setProperty("roundToIconSize", false);
> +
Just checking the values here is not enough, the property change results in
changes in paintedWidth, but we're currently not signalling that these props
have changed. See also my comment below.
> iconitem.cpp:313
> +}
> +void IconItem::setRoundToIconSize(bool roundToIconSize)
> +{
Changing roundToIconSize may change the size of the icon, so should we trigger
size changes (paintedWidth / paintedHeight / boundingRect likely? Perhaps
others.) and re-rendering here as well? Right now, judging from the code,
changing this property mid-flight is broken. We may even have to go as far as
loading a new pixmap (in loadPixmap() from a quick glance).
Please also add unit tests for the effects on the iconitem's size properties.
> iconitem.h:147
>
> +bool isRoundingToIconSize() const;
> +void setRoundToIconSize(bool roundToIconSize);
bool roundToIconSize() const;
we generally don't use "is" in new code where we can avoid it, and the ing
makes this even more inconsistent. I'd much prefer the above suggestion.
Please also add api docs, at least for new code.
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D4689
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: drosca, #plasma, sebas
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff,
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol
[Differential] [Accepted] D4572: Migrate AppearAnimation and DisappearAnimation to Animators
sebas accepted this revision. sebas added a reviewer: sebas. This revision is now accepted and ready to land. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D4572 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: broulik, #plasma, sebas Cc: plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
Re: Trusting .desktop files
On Saturday, February 11, 2017 7:24:11 AM UTC Martin Gräßlin wrote: > What I don't like in general is that this is all happening as $user. > Thus any malicious program running as $user can also just change the > list of trusted Exec= values. > > So my suggestion is: let's use polkit. > > The list of trusted .desktop files must be root owned and per user. > Everytime a user asks for executing a not known (or changed) desktop > file, it goes through polkit. To detect changes of the desktop file I > would suggest to store the shasum of the desktop file in addition. This > would prevent malicious programs to just change the desktop file. > > What do you think? Sensible? Too much? I like the approach, though it does sound a bit like overkill. But then, going the extra mile to improve security is right within our mission, so I think the approach is feasible, as it provides a lot of value for what we regard as our core competence. I can imagine this mechanism to be useful for other things as well, such as scripts, binaries and such that are user-writable. -- sebas http://www.kde.org • http://vizZzion.org
Re: Review Request 122732: Add std::function overloads for KServiceTypeTrader
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122732/#review102447 --- Is this still relevant? If not, please discard it... - Sebastian Kügler On Feb. 26, 2015, 7:44 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122732/ > --- > > (Updated Feb. 26, 2015, 7:44 p.m.) > > > Review request for KDE Frameworks, Marco Martin and Sebastian Kügler. > > > Repository: kservice > > > Description > --- > > These are a lot more flexible and less error-prone and will eventually > allow us to remove the trader query language in KF6 once all users in > KService are gone. > > REVIEW: 122732 > > > Diffs > - > > autotests/kservicetest.cpp d46f868185c3bf45138d80d04f4eb0d2840de9ca > autotests/ksycocathreadtest.cpp fbd889b28a32397fbf9245827ff8b54405b82e3d > src/services/kservicetypetrader.h 8e46812c2eeddca225e978a4dd55aa4cc5e902d0 > src/services/kservicetypetrader.cpp > 290e44e9161c8db47278543714426fdd3b5a87af > > Diff: https://git.reviewboard.kde.org/r/122732/diff/ > > > Testing > --- > > Unit tests pass > > There are still some more classes that use the string based checks, I'll add > a std::function overload to them as well once this has been approved. > > > Thanks, > > Alex Richardson > >
Re: All Frameworks modules are now available as repositories on Phabricator
On Tuesday, December 27, 2016 8:56:00 PM UTC Luigi Toscano wrote: > Elvis Angelaccio ha scritto: > > On Mon, Dec 26, 2016 at 1:19 AM, Luigi Toscano <luigi.tosc...@tiscali.it> wrote: > >> ... so that it's easier to use Differential for reviews. Much appreciated, thanks! > > Thanks! > > What about tasks? Should we create a Workboard for the Frameworks > > project? Or is it planned that each framework be a Project with its > > own workboard? > > This is entirely up to each maintainer. I personally think that 80% of the > modules in Frameworks don't need a separate project for each of them, but > again, if someone needs a separate project (and workboard), sure, it can be > created. Note that tasks can be on more than one workboard at the same time (they even carry status per workboard). So we can have a frameworks board with all tasks and project-specific board with just the relevant tasks, for example. -- sebas Sebastian Kügler•http://vizZzion.org•http://www.kde.org
Re: Review Request 129607: Display Application version in About dialog header
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129607/#review101330 --- Ship it! Ship It! - Sebastian Kügler On Dec. 7, 2016, 11:31 a.m., Jean-Baptiste Mardelle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129607/ > --- > > (Updated Dec. 7, 2016, 11:31 a.m.) > > > Review request for KDE Frameworks, KDE Usability and Sebastian Kügler. > > > Repository: kxmlgui > > > Description > --- > > Last year, a change in kaboutapplicationdialog (review 124133) moved the > application version in a separate tab. However for applications, it is really > annoying (several Kdenlive users complained and I fully agree) to not have > this information clearly displayed as soon as the About dialog is displayed. > I understand the interest of having a separate version tab displaying the > various underlying libraries used, but I propose to display again the > application version in the About dialog header (see screenshot). > > > Diffs > - > > src/kaboutapplicationdialog.cpp 156410e > > Diff: https://git.reviewboard.kde.org/r/129607/diff/ > > > Testing > --- > > Reverts a recent change, tested and works. > > > File Attachments > > > About dialog with version > > https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png > Updated patch > > https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/ee13cb47-2162-4be7-8255-3dce7f470c2b__about2.png > renamed to libraries > > https://git.reviewboard.kde.org/media/uploaded/files/2016/12/07/f01ed0c3-1391-48cf-a153-a177d9a7b54a__about3.png > > > Thanks, > > Jean-Baptiste Mardelle > >
Re: Review Request 129588: http slave: add newlines to long error message
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129588/#review101296 --- Ship it! Ship It! - Sebastian Kügler On Nov. 29, 2016, 6:23 p.m., Elvis Angelaccio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129588/ > --- > > (Updated Nov. 29, 2016, 6:23 p.m.) > > > Review request for KDE Frameworks and David Faure. > > > Repository: kio > > > Description > --- > > Otherwise Dolphin shows an error dialog with too much width. > > > Diffs > - > > src/ioslaves/http/http.cpp 62eb09d2882097c3dea99ea73fd1933cea4376a7 > > Diff: https://git.reviewboard.kde.org/r/129588/diff/ > > > Testing > --- > > See screenshots. > > > File Attachments > > > Before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/11/29/bb368736-5987-4800-996f-2e05c4971d7f__Spectacle.a29637.png > After > > https://git.reviewboard.kde.org/media/uploaded/files/2016/11/29/456f0afe-d192-4e09-8c9a-1fa0b27f5bb4__6wJEi10.png > > > Thanks, > > Elvis Angelaccio > >
Re: Review Request 129607: Display Application version in About dialog header
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129607/#review101235 --- I understand the problem, but I don't like the duplication you're introducing with this patch: The version number is now on the screen two times when you open the versions tab, and also, why have a versions tab, when the version is already displayed. - Sebastian Kügler On Dec. 3, 2016, 12:51 p.m., Jean-Baptiste Mardelle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129607/ > --- > > (Updated Dec. 3, 2016, 12:51 p.m.) > > > Review request for KDE Frameworks, KDE Usability and Sebastian Kügler. > > > Repository: kxmlgui > > > Description > --- > > Last year, a change in kaboutapplicationdialog (review 124133) moved the > application version in a separate tab. However for applications, it is really > annoying (several Kdenlive users complained and I fully agree) to not have > this information clearly displayed as soon as the About dialog is displayed. > I understand the interest of having a separate version tab displaying the > various underlying libraries used, but I propose to display again the > application version in the About dialog header (see screenshot). > > > Diffs > - > > src/kaboutapplicationdialog.cpp 156410e > > Diff: https://git.reviewboard.kde.org/r/129607/diff/ > > > Testing > --- > > Reverts a recent change, tested and works. > > > File Attachments > > > About dialog with version > > https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png > > > Thanks, > > Jean-Baptiste Mardelle > >
Re: Review Request 126226: kdetemplate_add_app_templates installs previews
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126226/ --- (Updated Dec. 2, 2016, 12:13 p.m.) Status -- This change has been discarded. Review request for KDE Frameworks and Marco Martin. Repository: extra-cmake-modules Description --- kapptemplate can't deal with templats without previews, so make sure we install the preview image. Without this patch, the installed templates show up broken in kapptemplate. With it, they work as expected. I'm assuming here that the preview has the baseName + .png, otherwise we'd have to read Icon field from the .kdevtemplate file, but that seems way too much hassle. The error message resulting from a wrong file name will show the expected filename, so it doesn't exactly hide the error. Diffs - kde-modules/KDETemplateMacro.cmake 796c3f1 Diff: https://git.reviewboard.kde.org/r/126226/diff/ Testing --- Installed templates/ from plasma-framework, this patch makes them work in kapptemplate. Thanks, Sebastian Kügler
Re: Review Request 129598: fix build, needs QtDBus
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129598/ --- (Updated Dec. 2, 2016, 11:09 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 04389b36653b2d0df8a3ac2fc62bafa1b86e by Sebastian Kügler to branch master. Repository: frameworkintegration Description --- The new appstream kpackage handler needs to link Qt DBus, but the find package call is missing. Diffs - CMakeLists.txt e97e698 Diff: https://git.reviewboard.kde.org/r/129598/diff/ Testing --- Builds and works on my machine Thanks, Sebastian Kügler
Review Request 129598: fix build, needs QtDBus
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129598/ --- Review request for KDE Frameworks. Repository: frameworkintegration Description --- The new appstream kpackage handler needs to link Qt DBus, but the find package call is missing. Diffs - CMakeLists.txt e97e698 Diff: https://git.reviewboard.kde.org/r/129598/diff/ Testing --- Builds and works on my machine Thanks, Sebastian Kügler
[Differential] [Accepted] D3314: Properly test environment variable
sebas accepted this revision. sebas added a reviewer: sebas. This revision is now accepted and ready to land. BRANCH master REVISION DETAIL https://phabricator.kde.org/D3314 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: apol, #frameworks, sebas
Re: Review Request 129118: Remove discovery associated to a key when removing a definition
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129118/#review99839
---
Ship it!
Ship It!
- Sebastian Kügler
On Oct. 7, 2016, 4:10 p.m., Antonio Larrosa Jimenez wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129118/
> ---
>
> (Updated Oct. 7, 2016, 4:10 p.m.)
>
>
> Review request for KDE Frameworks and Plasma.
>
>
> Repository: kpackage
>
>
> Description
> ---
>
> Plasmashell was calling Package::removeDefinition but a later call to
> Package::filePath still returned a value because there was an entry in
> d->discoveries but no way to remove it, which made plasma use old
> values.
>
>
> Diffs
> -
>
> src/kpackage/package.cpp 363a67ed5b7c592aea4845b84ca05e63da05991a
>
> Diff: https://git.reviewboard.kde.org/r/129118/diff/
>
>
> Testing
> ---
>
> I gdb'd a patched plasmashell (1) to fix a wallpaper aspect ratio problem
> while changing the screen resolution between 1024x768 and 1280x720
> and tracing Image::findPreferedImageInPackage .
>
> package.filePath("preferred") is not empty at the beginning of the method
> even if package.removeDefinition("preferred") was just called, so it seems
> the key should be removed from d->discoveries too.
>
> (1) Plasma patch is comming to phabricator in the next minutes.
>
>
> Thanks,
>
> Antonio Larrosa Jimenez
>
>
Re: Review Request 128740: uncompress archives in subfolders
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128740/#review98579
---
Ship it!
Tested and confirmed working. Thanks, Marco!
- Sebastian Kügler
On Aug. 23, 2016, 2:43 p.m., Marco Martin wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128740/
> ---
>
> (Updated Aug. 23, 2016, 2:43 p.m.)
>
>
> Review request for KDE Frameworks, Sebastian Kügler and Jeremy Whiting.
>
>
> Repository: knewstuff
>
>
> Description
> ---
>
> if the archive downloaded by ghns has more than a file inside (either single
> file or a folder at the root), uncompress it in a folder called the same as
> the file name (without path or extension)
>
> this fixes downloading and installing plasma wallpaper packages (those that
> provide multiple resolutions).
> I don't know if this behavior would in turn break other applications, but i
> don't think so as it seems the behavior changed between kde4 and kf5.
> if needed i can make it do this only in the case of wallpapers (if
> (standardResourceDirectory == QLatin1String("wallpaper"))) but i think this
> should be the correct behavior
>
>
> Diffs
> -
>
> src/core/installation.cpp a027418
>
> Diff: https://git.reviewboard.kde.org/r/128740/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Marco Martin
>
>
Re: Review Request 128641: Fix argument type in QMetaObject invoke *NEEDS BACKPORTING TO 5.25*
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128641/#review98253 --- Ship it! Ship It! - Sebastian Kügler On Aug. 9, 2016, 11:04 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128641/ > --- > > (Updated Aug. 9, 2016, 11:04 p.m.) > > > Review request for KDE Frameworks, Plasma and David Faure. > > > Repository: plasma-framework > > > Description > --- > > Commit bb70febdbe397b617e5c41aff5494fdbc185fa88 ported an argument from > QPoint to QRectF. However, not all local invocations were updated correctly > with one passing a QRect, which fails. > > *NEEDS BACKPORTING TO 5.25* > > > Diffs > - > > src/scriptengines/qml/plasmoid/containmentinterface.cpp > 4efc1e109bbab8ef43b686be25574ed5bd66d9ce > > Diff: https://git.reviewboard.kde.org/r/128641/diff/ > > > Testing > --- > > Before: Couldn't drag applet from Widget Explorer to Desktop > After: Can > > Checked the other invocations are correct. > > > Thanks, > > David Edmundson > >
Re: Review Request 128640: Keep compatiable slot createApplet with Frameworks 5.24 *THIS NEEDS BACKPORTING TO 5.25*
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128640/#review98252 --- Ship it! Ship It! - Sebastian Kügler On Aug. 9, 2016, 11:03 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128640/ > --- > > (Updated Aug. 9, 2016, 11:03 p.m.) > > > Review request for KDE Frameworks, Plasma and David Faure. > > > Repository: plasma-framework > > > Description > --- > > Commit bb70febdbe397b617e5c41aff5494fdbc185fa88 changed the slot for > adding createApplet, turning the final argument from QPoint to QRect. > > If the rectangle size is nothing, it acts like the original code. > > Despite this being private API (ish) there is a hacks in plasma- > workspace that call methods on the view via QMetaObject invoke. This > obviously fails. As we need compatibility for Plasma/5.7 and earlier a > compatibility slot needs to stay. > > > *THIS NEEDS BACKPORTING TO 5.25* > > > Diffs > - > > src/scriptengines/qml/plasmoid/containmentinterface.h > c058f8358b4aa123749959a3de5b0667d7a1fecc > src/scriptengines/qml/plasmoid/containmentinterface.cpp > 4efc1e109bbab8ef43b686be25574ed5bd66d9ce > > Diff: https://git.reviewboard.kde.org/r/128640/diff/ > > > Testing > --- > > Before: alternatives context menu on the desktop just deleted the applet > Now: works > > > Thanks, > > David Edmundson > >
Re: Review Request 128594: Add a kapptemplate for Plasma Wallpaper
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128594/#review98138 --- Ship it! Ship It! - Sebastian Kügler On Aug. 3, 2016, 5:05 p.m., Friedrich W. H. Kossebau wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128594/ > --- > > (Updated Aug. 3, 2016, 5:05 p.m.) > > > Review request for KDE Frameworks, Plasma and Marco Martin. > > > Repository: plasma-framework > > > Description > --- > > This template should allow people to get a basic working wallpaper plugin > within minutes. > > > Patch (in separate commits) also > - updates the READMEs of the plasmoid templates to point to correct QML2 wiki > pages. > - mobves the plasmoid and wallpaper templates into own toplevel category > "Plasma/", out of "KDE/", given this is a platform target of its own > > > Diffs > - > > templates/CMakeLists.txt b51777d > templates/cpp-plasmoid/README d3582db > templates/cpp-plasmoid/cpp-plasmoid.kdevtemplate 42924b0 > templates/plasma-wallpaper/CMakeLists.txt PRE-CREATION > templates/plasma-wallpaper/Messages.sh PRE-CREATION > templates/plasma-wallpaper/README PRE-CREATION > templates/plasma-wallpaper/package/contents/config/main.xml PRE-CREATION > templates/plasma-wallpaper/package/contents/ui/config.qml PRE-CREATION > templates/plasma-wallpaper/package/contents/ui/main.qml PRE-CREATION > templates/plasma-wallpaper/package/metadata.desktop PRE-CREATION > templates/plasma-wallpaper/plasma-wallpaper.kdevtemplate PRE-CREATION > templates/qml-plasmoid/README 6814263 > templates/qml-plasmoid/qml-plasmoid.kdevtemplate 2675e71 > > Diff: https://git.reviewboard.kde.org/r/128594/diff/ > > > Testing > --- > > Installed the template, then created a new wallpaper plugin using > kapptemplate and following the README, then selected in wallpaper config, > incl. editing the plugin config by entering a new text. > > > Thanks, > > Friedrich W. H. Kossebau > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF > > Sebastian Kügler wrote: > This looks logical to me, and it's simpler: Very good! > > (Take that as "sebas withdraws his objection" :)) > > Thomas Pfeiffer wrote: > Clear -1 to removing the group box. > > Here is tha rationale: > For most "regular" users, only the lineedit at the top is relevant. The > calculate stuff is just distraction and - worse - potential confusion. If we > remove any visual distinction between the two, we just make the situation > worse for them. > > I agree that calling the tab "Checksums" is still better, though, because > "Integrity" is too vague. > > For the "average user" I just re-tested this with, what would actually > help her is creating a second box around the verification part, calling the > top one "Verify checksum" and the bottom "Calculate checksums". > That way if she was told e.g. by a website to verify a checksum, she'd > knew she could simply ignore the whole calculation part. > > Overall simplicity should not be the top priority here: The priority > should be to make it clear to users who just want to check whether a download > went okay which part they should care about and which they can ignore, while > still providing the calculation part for advanced users who want to do that. > > Of course another option would be to split it in two tabs, but that might > make the tab bar quite long especially in languages like German. > > Sebastian Kügler wrote: > The latter part seems redundant then. How can you verify a checksum > without calculating it? > > Thomas Pfeiffer wrote: > See _that_ is why the bottom box was originally called "Share". Of course > the verification part also does calculation, but it does so without you > telling it to. You paste in the checksum, and it tells you whether it matches > or not. > The manual calculation at the bottom is for people who share a file with > others and want to accompany it with a checksum for _them_ to verify it. > > I think we might get away with "Calculate" anyway because those who don#t > know much about checksums don't need to know that the verify part calculates > as well, and those who know it should still be able to use the thing > correctly. > > Sebastian Kügler wrote: > That 'Share' title completely puzzled me, and I think I'm the kind of > user this dialog should work for very well. (I need to verify checksums all > the time.) > > To be quite honest, from getting it explained, I get the strong > impression that you're overthinking it. > > To me, the most logical would be: > > * Calculate checksums at the top > * Under that, the input field so I can c/p or type my checksum in there > to have it compared automatically. > > That's both, the order of the workflow as well as the logical order of > operation. 'Calculate' underneath would raise exactly the same question as I > put above: "...but but but ... it could not verify it without calculating it, > yet I have to hit a button to calculate ... maybe I should try again and > maybe I should just use the commandline to be sure". > > Point in case: for this kind of stuff, simplicity trumps since it makes > it easier to TRUST the dialog. I can't trust anything I don't fully
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF > > Sebastian Kügler wrote: > This looks logical to me, and it's simpler: Very good! > > (Take that as "sebas withdraws his objection" :)) > > Thomas Pfeiffer wrote: > Clear -1 to removing the group box. > > Here is tha rationale: > For most "regular" users, only the lineedit at the top is relevant. The > calculate stuff is just distraction and - worse - potential confusion. If we > remove any visual distinction between the two, we just make the situation > worse for them. > > I agree that calling the tab "Checksums" is still better, though, because > "Integrity" is too vague. > > For the "average user" I just re-tested this with, what would actually > help her is creating a second box around the verification part, calling the > top one "Verify checksum" and the bottom "Calculate checksums". > That way if she was told e.g. by a website to verify a checksum, she'd > knew she could simply ignore the whole calculation part. > > Overall simplicity should not be the top priority here: The priority > should be to make it clear to users who just want to check whether a download > went okay which part they should care about and which they can ignore, while > still providing the calculation part for advanced users who want to do that. > > Of course another option would be to split it in two tabs, but that might > make the tab bar quite long especially in languages like German. > > Sebastian Kügler wrote: > The latter part seems redundant then. How can you verify a checksum > without calculating it? > > Thomas Pfeiffer wrote: > See _that_ is why the bottom box was originally called "Share". Of course > the verification part also does calculation, but it does so without you > telling it to. You paste in the checksum, and it tells you whether it matches > or not. > The manual calculation at the bottom is for people who share a file with > others and want to accompany it with a checksum for _them_ to verify it. > > I think we might get away with "Calculate" anyway because those who don#t > know much about checksums don't need to know that the verify part calculates > as well, and those who know it should still be able to use the thing > correctly. > > Sebastian Kügler wrote: > That 'Share' title completely puzzled me, and I think I'm the kind of > user this dialog should work for very well. (I need to verify checksums all > the time.) > > To be quite honest, from getting it explained, I get the strong > impression that you're overthinking it. > > To me, the most logical would be: > > * Calculate checksums at the top > * Under that, the input field so I can c/p or type my checksum in there > to have it compared automatically. > > That's both, the order of the workflow as well as the logical order of > operation. 'Calculate' underneath would raise exactly the same question as I > put above: "...but but but ... it could not verify it without calculating it, > yet I have to hit a button to calculate ... maybe I should try again and > maybe I should just use the commandline to be sure". > > Point in case: for this kind of stuff, simplicity trumps since it makes > it easier to TRUST the dialog. I can't trust anything I don't fully
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF > > Sebastian Kügler wrote: > This looks logical to me, and it's simpler: Very good! > > (Take that as "sebas withdraws his objection" :)) > > Thomas Pfeiffer wrote: > Clear -1 to removing the group box. > > Here is tha rationale: > For most "regular" users, only the lineedit at the top is relevant. The > calculate stuff is just distraction and - worse - potential confusion. If we > remove any visual distinction between the two, we just make the situation > worse for them. > > I agree that calling the tab "Checksums" is still better, though, because > "Integrity" is too vague. > > For the "average user" I just re-tested this with, what would actually > help her is creating a second box around the verification part, calling the > top one "Verify checksum" and the bottom "Calculate checksums". > That way if she was told e.g. by a website to verify a checksum, she'd > knew she could simply ignore the whole calculation part. > > Overall simplicity should not be the top priority here: The priority > should be to make it clear to users who just want to check whether a download > went okay which part they should care about and which they can ignore, while > still providing the calculation part for advanced users who want to do that. > > Of course another option would be to split it in two tabs, but that might > make the tab bar quite long especially in languages like German. > > Sebastian Kügler wrote: > The latter part seems redundant then. How can you verify a checksum > without calculating it? > > Thomas Pfeiffer wrote: > See _that_ is why the bottom box was originally called "Share". Of course > the verification part also does calculation, but it does so without you > telling it to. You paste in the checksum, and it tells you whether it matches > or not. > The manual calculation at the bottom is for people who share a file with > others and want to accompany it with a checksum for _them_ to verify it. > > I think we might get away with "Calculate" anyway because those who don#t > know much about checksums don't need to know that the verify part calculates > as well, and those who know it should still be able to use the thing > correctly. > > Sebastian Kügler wrote: > That 'Share' title completely puzzled me, and I think I'm the kind of > user this dialog should work for very well. (I need to verify checksums all > the time.) > > To be quite honest, from getting it explained, I get the strong > impression that you're overthinking it. > > To me, the most logical would be: > > * Calculate checksums at the top > * Under that, the input field so I can c/p or type my checksum in there > to have it compared automatically. > > That's both, the order of the workflow as well as the logical order of > operation. 'Calculate' underneath would raise exactly the same question as I > put above: "...but but but ... it could not verify it without calculating it, > yet I have to hit a button to calculate ... maybe I should try again and > maybe I should just use the commandline to be sure". > > Point in case: for this kind of stuff, simplicity trumps since it makes > it easier to TRUST the dialog. I can't trust anything I don't fully
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF > > Sebastian Kügler wrote: > This looks logical to me, and it's simpler: Very good! > > (Take that as "sebas withdraws his objection" :)) > > Thomas Pfeiffer wrote: > Clear -1 to removing the group box. > > Here is tha rationale: > For most "regular" users, only the lineedit at the top is relevant. The > calculate stuff is just distraction and - worse - potential confusion. If we > remove any visual distinction between the two, we just make the situation > worse for them. > > I agree that calling the tab "Checksums" is still better, though, because > "Integrity" is too vague. > > For the "average user" I just re-tested this with, what would actually > help her is creating a second box around the verification part, calling the > top one "Verify checksum" and the bottom "Calculate checksums". > That way if she was told e.g. by a website to verify a checksum, she'd > knew she could simply ignore the whole calculation part. > > Overall simplicity should not be the top priority here: The priority > should be to make it clear to users who just want to check whether a download > went okay which part they should care about and which they can ignore, while > still providing the calculation part for advanced users who want to do that. > > Of course another option would be to split it in two tabs, but that might > make the tab bar quite long especially in languages like German. > > Sebastian Kügler wrote: > The latter part seems redundant then. How can you verify a checksum > without calculating it? > > Thomas Pfeiffer wrote: > See _that_ is why the bottom box was originally called "Share". Of course > the verification part also does calculation, but it does so without you > telling it to. You paste in the checksum, and it tells you whether it matches > or not. > The manual calculation at the bottom is for people who share a file with > others and want to accompany it with a checksum for _them_ to verify it. > > I think we might get away with "Calculate" anyway because those who don#t > know much about checksums don't need to know that the verify part calculates > as well, and those who know it should still be able to use the thing > correctly. That 'Share' title completely puzzled me, and I think I'm the kind of user this dialog should work for very well. (I need to verify checksums all the time.) To be quite honest, from getting it explained, I get the strong impression that you're overthinking it. To me, the most logical would be: * Calculate checksums at the top * Under that, the input field so I can c/p or type my checksum in there to have it compared automatically. That's both, the order of the workflow as well as the logical order of operation. 'Calculate' underneath would raise exactly the same question as I put above: "...but but but ... it could not verify it without calculating it, yet I have to hit a button to calculate ... maybe I should try again and maybe I should just use the commandline to be sure". Point in case: for this kind of stuff, simplicity trumps since it makes it easier to TRUST the dialog. I can't trust anything I don't fully understand or have doubts about, and that's what the groupbox design and the share title cause: doubts. - Sebastian --
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF > > Sebastian Kügler wrote: > This looks logical to me, and it's simpler: Very good! > > (Take that as "sebas withdraws his objection" :)) > > Thomas Pfeiffer wrote: > Clear -1 to removing the group box. > > Here is tha rationale: > For most "regular" users, only the lineedit at the top is relevant. The > calculate stuff is just distraction and - worse - potential confusion. If we > remove any visual distinction between the two, we just make the situation > worse for them. > > I agree that calling the tab "Checksums" is still better, though, because > "Integrity" is too vague. > > For the "average user" I just re-tested this with, what would actually > help her is creating a second box around the verification part, calling the > top one "Verify checksum" and the bottom "Calculate checksums". > That way if she was told e.g. by a website to verify a checksum, she'd > knew she could simply ignore the whole calculation part. > > Overall simplicity should not be the top priority here: The priority > should be to make it clear to users who just want to check whether a download > went okay which part they should care about and which they can ignore, while > still providing the calculation part for advanced users who want to do that. > > Of course another option would be to split it in two tabs, but that might > make the tab bar quite long especially in languages like German. The latter part seems redundant then. How can you verify a checksum without calculating it? - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128466/#review97521 --- On July 16, 2016, 12:35 p.m., Elvis Angelaccio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128466/ > --- > > (Updated July 16, 2016, 12:35 p.m.) > > > Review request for KDE Frameworks, KDE Usability and Dominik Haumann. > > > Repository: kio > > > Description > --- > > Dominik suggested to rename the `Checksums` tab to `Integrity`, so that we > can "free" the Checksums string and use it as the title of the groupbox below > (in place of the current `Share` string, which can be confusing). > > Preview in the attached screenshot. > > > Diffs > - > > src/widgets/checksumswidget.ui 03c64db > src/widgets/kpropertiesdialog.cpp 808765c > > Diff: https://git.reviewboard.kde.org/r/128466/diff/ > > > Testing > --- > > > File Attachments > > > Before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/6771ed06-c803-4d18-abe3-91e4f97c8c76__checksums-tab.png > After > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/b2cd12c8-6bbf-4123-9e8e-59cb0c29cbdb__Spectacle.TJ7614.png > > > Thanks, > > Elvis Angelaccio > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... > > Sebastian Kügler wrote: > It has changed in a significant way, though, making it illogical. > > (Not that I understand the "Share" title in the original review, but > that's another matter.) > > This needs more work. > > Elvis Angelaccio wrote: > > Perhaps the whole thing could be simplified by naming the tab > "Checksums" and removing the groupbox altogether, as it's not providing any > semantic value. > > Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF This looks logical to me, and it's simpler: Very good! (Take that as "sebas withdraws his objection" :)) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128466/#review97521 --- On July 16, 2016, 12:35 p.m., Elvis Angelaccio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128466/ > --- > > (Updated July 16, 2016, 12:35 p.m.) > > > Review request for KDE Frameworks, KDE Usability and Dominik Haumann. > > > Repository: kio > > > Description > --- > > Dominik suggested to rename the `Checksums` tab to `Integrity`, so that we > can "free" the Checksums string and use it as the title of the groupbox below > (in place of the current `Share` string, which can be confusing). > > Preview in the attached screenshot. > > > Diffs > - > > src/widgets/checksumswidget.ui 03c64db > src/widgets/kpropertiesdialog.cpp 808765c > > Diff: https://git.reviewboard.kde.org/r/128466/diff/ > > > Testing > --- > > > File Attachments > > > Before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/6771ed06-c803-4d18-abe3-91e4f97c8c76__checksums-tab.png > After > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/b2cd12c8-6bbf-4123-9e8e-59cb0c29cbdb__Spectacle.TJ7614.png > > > Thanks, > > Elvis Angelaccio > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 128466: Rename Checksums tab to Integrity
> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote: > > Please don't ship it, yet. > > > > > > I find the UI illogical. There's a groupbox grouping the checksum buttons, > > but then you can input the checksum above, so essentially, the groupbox is > > unnecessary and confusing. > > > > Perhaps the whole thing could be simplified by naming the tab "Checksums" > > and removing the groupbox altogether, as it's not providing any semantic > > value. > > > > A usability reviewer should have a look. > > Kai Uwe Broulik wrote: > This dialog has been created in Review 128283 and Usability has been > involved from the beginning... It has changed in a significant way, though, making it illogical. (Not that I understand the "Share" title in the original review, but that's another matter.) This needs more work. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128466/#review97521 --- On July 16, 2016, 12:35 p.m., Elvis Angelaccio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128466/ > --- > > (Updated July 16, 2016, 12:35 p.m.) > > > Review request for KDE Frameworks, KDE Usability and Dominik Haumann. > > > Repository: kio > > > Description > --- > > Dominik suggested to rename the `Checksums` tab to `Integrity`, so that we > can "free" the Checksums string and use it as the title of the groupbox below > (in place of the current `Share` string, which can be confusing). > > Preview in the attached screenshot. > > > Diffs > - > > src/widgets/checksumswidget.ui 03c64db > src/widgets/kpropertiesdialog.cpp 808765c > > Diff: https://git.reviewboard.kde.org/r/128466/diff/ > > > Testing > --- > > > File Attachments > > > Before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/6771ed06-c803-4d18-abe3-91e4f97c8c76__checksums-tab.png > After > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/b2cd12c8-6bbf-4123-9e8e-59cb0c29cbdb__Spectacle.TJ7614.png > > > Thanks, > > Elvis Angelaccio > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 128466: Rename Checksums tab to Integrity
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128466/#review97521 --- Please don't ship it, yet. I find the UI illogical. There's a groupbox grouping the checksum buttons, but then you can input the checksum above, so essentially, the groupbox is unnecessary and confusing. Perhaps the whole thing could be simplified by naming the tab "Checksums" and removing the groupbox altogether, as it's not providing any semantic value. A usability reviewer should have a look. - Sebastian Kügler On July 16, 2016, 12:35 p.m., Elvis Angelaccio wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128466/ > --- > > (Updated July 16, 2016, 12:35 p.m.) > > > Review request for KDE Frameworks, KDE Usability and Dominik Haumann. > > > Repository: kio > > > Description > --- > > Dominik suggested to rename the `Checksums` tab to `Integrity`, so that we > can "free" the Checksums string and use it as the title of the groupbox below > (in place of the current `Share` string, which can be confusing). > > Preview in the attached screenshot. > > > Diffs > - > > src/widgets/checksumswidget.ui 03c64db > src/widgets/kpropertiesdialog.cpp 808765c > > Diff: https://git.reviewboard.kde.org/r/128466/diff/ > > > Testing > --- > > > File Attachments > > > Before > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/6771ed06-c803-4d18-abe3-91e4f97c8c76__checksums-tab.png > After > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/16/b2cd12c8-6bbf-4123-9e8e-59cb0c29cbdb__Spectacle.TJ7614.png > > > Thanks, > > Elvis Angelaccio > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 128085: Fix check that CPU is a valid CPU
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128085/#review96263 --- Ship it! - Sebastian Kügler On June 3, 2016, 12:15 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128085/ > --- > > (Updated June 3, 2016, 12:15 p.m.) > > > Review request for KDE Frameworks and Solid. > > > Repository: solid > > > Description > --- > > We have a check that an devices/system/cpu/cpuN entry contains a > processor and not just an empty socket, however it relies on somewhat > outdated /sys files. > > cpuN/cpufreq isn't guaranteed to exist as for some systems (my AMD > processor at least). > > The docs in the linux kernel imply topology/core_id should always exist, > and should still work as a valid check that we have a populated CPU > socket. > > > Diffs > - > > src/solid/devices/backends/udev/udevmanager.cpp > 3f3a671798e84e6d577df7c3b9b80150ac4d01fc > > Diff: https://git.reviewboard.kde.org/r/128085/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127870: plasma-framework: AppletInterface::downloadPath()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127870/#review95815 --- Ship it! ping :) - Sebastian Kügler On May 8, 2016, 4:15 p.m., Allen Winter wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127870/ > --- > > (Updated May 8, 2016, 4:15 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > Deprecate downloadPath(const QString ) in favor of downloadPath() since > the file argument is not used. > > > Diffs > - > > src/scriptengines/qml/plasmoid/appletinterface.h 6cf71aa > src/scriptengines/qml/plasmoid/appletinterface.cpp 876af2c > > Diff: https://git.reviewboard.kde.org/r/127870/diff/ > > > Testing > --- > > compiles. make test still passes. > > > Thanks, > > Allen Winter > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127842: take highlight and highlightedText from proper color group
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127842/#review95200 --- Ship it! Ship It! - Sebastian Kügler On May 5, 2016, 9:54 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127842/ > --- > > (Updated May 5, 2016, 9:54 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > lately the highlight color of plasma became a very washed-out blue, (since it > switched to use the global system colors) that was because it actually taken > the highlight color (and highlightedText) from the wrong source. > there is a correct color group in kcolorscheme for that, that is Selection, > take those two colors from that (also means highlight and highlightedtext > always be the same in all plasma::theme::colorgroups but that's not a problem) > > update test to reflect that > > > Diffs > - > > autotests/themetest.cpp ce512a4 > src/declarativeimports/core/quicktheme.h ac5e121 > src/plasma/private/theme_p.h d2246de > src/plasma/private/theme_p.cpp e6d55c3 > > Diff: https://git.reviewboard.kde.org/r/127842/diff/ > > > Testing > --- > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127779: use system colors for monochrome icons
> On May 2, 2016, 1:17 p.m., Sebastian Kügler wrote: > > src/kiconloader.cpp, line 869 > > <https://git.reviewboard.kde.org/r/127779/diff/9/?file=461529#file461529line869> > > > > Where does this one get deleted? > > Marco Martin wrote: > it's a QScopedPointer, so it gets deleted when the function returns Ah right ... sorry for being blind. :) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127779/#review95076 --- On May 4, 2016, 9:56 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127779/ > --- > > (Updated May 4, 2016, 9:56 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kiconthemes > > > Description > --- > > Breeze uses stylesheet information to color its icons with some "named" > colors that change depending from the current system color scheme, this is > already used since some time in the icons used by the Plasma shell. > But QWidget applications have the same problem, when the user changes the > color scheme from breeze to breeze dark (or any color scheme), most of the > icons will be black on black. > This patch clones (a bit simplified and taking only the most "important" > colors) the logic used by Plasma::Svg to color icons with the stylesheet. > > even tough it's doing more things at icon generation, an application that > uses a lot of icons like Dolphin doesn't seem to have noticeable startup time > difference, even when the image cache is not present yet, so i hope is not an > unacceptable performance tradeoff (successive loads are unchanged as are from > the image cache). > > still some questions and things that can be optimized, like > > * an optional key in the theme metadata file to explicitly enable this, to > not have it running in themes that don't care? > > * can i use karchive in this framework?, so it would work with svgz icons as > well > > * right now to refresh icons at runtime it depends from a patch in the colors > kcm to emit iconchanges as well, alternatively kiconloader could watch for > kcolorscheme changes as well, but i don't think is strictly necessary > > > Diffs > - > > CMakeLists.txt 2e838e8 > autotests/coloredsvgicon.svg PRE-CREATION > autotests/kiconloader_unittest.cpp 0e47cc8 > autotests/resources.qrc a19c963 > src/CMakeLists.txt 0e30a35 > src/kiconloader.cpp 75ab482 > src/kicontheme.h 3190665 > src/kicontheme.cpp 0996054 > > Diff: https://git.reviewboard.kde.org/r/127779/diff/ > > > Testing > --- > > > File Attachments > > > dadel1.png > > https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/0fc42425-947c-479e-9759-09c7a703a456__dadel1.png > Gwenview in fullscreen flips colors > > https://git.reviewboard.kde.org/media/uploaded/files/2016/05/04/1c0cab70-a54f-4533-b5e9-0b3af07ae087__dadel2.png > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127819: [autotests] Use -displayfd as argument to start Xvfb
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127819/#review95139 --- Ship it! Ship It! - Sebastian Kügler On May 3, 2016, 6:32 a.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127819/ > --- > > (Updated May 3, 2016, 6:32 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kwindowsystem > > > Description > --- > > Makes the test more reliable, our side blocks till the server is > fully started. > > The approach is used by KWin's autotest. > > > Diffs > - > > autotests/netrootinfotestwm.cpp 3816ce2c0ff4ca7a6c5b1c37532e10f0fafec63c > autotests/netwininfotestclient.cpp 222b5b1e959fa44bb2b242757ff32abf31b63be7 > autotests/netwininfotestwm.cpp 9670a14dcb8faebf2ac6af8d56ead3681fa11715 > > Diff: https://git.reviewboard.kde.org/r/127819/diff/ > > > Testing > --- > > > Thanks, > > Martin Gräßlin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127779: use system colors for monochrome icons
> On May 2, 2016, 1:09 p.m., Sebastian Kügler wrote: > > Okay, let's try this in master and see if it's going to cause problems. > > Kai Uwe Broulik wrote: > Beware this is frameworks and the next frameworks release will be > upcoming Saturday > > Marco Martin wrote: > I was aiming at first for this one. Do you think has potential issues > that would make it better having it in 5.23? I don't see any, but then, it's pretty intrusive. I'd trust your judgment, Marco. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127779/#review95075 --- On April 29, 2016, 9:25 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127779/ > --- > > (Updated April 29, 2016, 9:25 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kiconthemes > > > Description > --- > > Breeze uses stylesheet information to color its icons with some "named" > colors that change depending from the current system color scheme, this is > already used since some time in the icons used by the Plasma shell. > But QWidget applications have the same problem, when the user changes the > color scheme from breeze to breeze dark (or any color scheme), most of the > icons will be black on black. > This patch clones (a bit simplified and taking only the most "important" > colors) the logic used by Plasma::Svg to color icons with the stylesheet. > > even tough it's doing more things at icon generation, an application that > uses a lot of icons like Dolphin doesn't seem to have noticeable startup time > difference, even when the image cache is not present yet, so i hope is not an > unacceptable performance tradeoff (successive loads are unchanged as are from > the image cache). > > still some questions and things that can be optimized, like > > * an optional key in the theme metadata file to explicitly enable this, to > not have it running in themes that don't care? > > * can i use karchive in this framework?, so it would work with svgz icons as > well > > * right now to refresh icons at runtime it depends from a patch in the colors > kcm to emit iconchanges as well, alternatively kiconloader could watch for > kcolorscheme changes as well, but i don't think is strictly necessary > > > Diffs > - > > CMakeLists.txt 2e838e8 > autotests/coloredsvgicon.svg PRE-CREATION > autotests/kiconloader_unittest.cpp 0e47cc8 > autotests/resources.qrc a19c963 > src/CMakeLists.txt 0e30a35 > src/kiconloader.cpp 75ab482 > src/kicontheme.h 3190665 > src/kicontheme.cpp 0996054 > > Diff: https://git.reviewboard.kde.org/r/127779/diff/ > > > Testing > --- > > > File Attachments > > > dadel1.png > > https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/0fc42425-947c-479e-9759-09c7a703a456__dadel1.png > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127810: Fix some gcc5.3 compile warnings in kwayland
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127810/#review95079 --- Ship it! - Sebastian Kügler On May 1, 2016, 8:32 p.m., Allen Winter wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127810/ > --- > > (Updated May 1, 2016, 8:32 p.m.) > > > Review request for KDE Frameworks, Plasma and Martin Gräßlin. > > > Repository: kwayland > > > Description > --- > > eliminate -Wmissing-include-dirs, -Wunused-parameter and -Wsign-compare > warnings from gcc5.3.1 > > > Diffs > - > > autotests/client/CMakeLists.txt ed0970b > src/client/outputdevice.cpp 6fc3d3c > src/client/outputmanagement.cpp 9aaf331 > src/client/plasmawindowmanagement.cpp 81c6a3b > > Diff: https://git.reviewboard.kde.org/r/127810/diff/ > > > Testing > --- > > still compiles. > a warning remains that I don't know how to fix yet: > /data/kde5/src/frameworks/kwayland/src/client/buffer_p.h:32:15: warning: > ‘KWayland::Client::Buffer::Private’ has a field > ‘KWayland::Client::Buffer::Private::nativeBuffer’ whose type uses the > anonymous namespace > > but otherwise most other compiler warnings are fixed with this patch > > > Thanks, > > Allen Winter > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127779: use system colors for monochrome icons
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127779/#review95076 --- autotests/kiconloader_unittest.cpp (line 441) <https://git.reviewboard.kde.org/r/127779/#comment64522> Has the image been recolored to red ... (word order makes it easier to read) src/kiconloader.cpp (line 290) <https://git.reviewboard.kde.org/r/127779/#comment64524> If the icon is an SVG file, ... src/kiconloader.cpp (line 292) <https://git.reviewboard.kde.org/r/127779/#comment64523> text (lower case), perhaps @see the colors that are available? src/kiconloader.cpp (line 869) <https://git.reviewboard.kde.org/r/127779/#comment64525> Where does this one get deleted? - Sebastian Kügler On April 29, 2016, 9:25 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127779/ > --- > > (Updated April 29, 2016, 9:25 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kiconthemes > > > Description > --- > > Breeze uses stylesheet information to color its icons with some "named" > colors that change depending from the current system color scheme, this is > already used since some time in the icons used by the Plasma shell. > But QWidget applications have the same problem, when the user changes the > color scheme from breeze to breeze dark (or any color scheme), most of the > icons will be black on black. > This patch clones (a bit simplified and taking only the most "important" > colors) the logic used by Plasma::Svg to color icons with the stylesheet. > > even tough it's doing more things at icon generation, an application that > uses a lot of icons like Dolphin doesn't seem to have noticeable startup time > difference, even when the image cache is not present yet, so i hope is not an > unacceptable performance tradeoff (successive loads are unchanged as are from > the image cache). > > still some questions and things that can be optimized, like > > * an optional key in the theme metadata file to explicitly enable this, to > not have it running in themes that don't care? > > * can i use karchive in this framework?, so it would work with svgz icons as > well > > * right now to refresh icons at runtime it depends from a patch in the colors > kcm to emit iconchanges as well, alternatively kiconloader could watch for > kcolorscheme changes as well, but i don't think is strictly necessary > > > Diffs > - > > CMakeLists.txt 2e838e8 > autotests/coloredsvgicon.svg PRE-CREATION > autotests/kiconloader_unittest.cpp 0e47cc8 > autotests/resources.qrc a19c963 > src/CMakeLists.txt 0e30a35 > src/kiconloader.cpp 75ab482 > src/kicontheme.h 3190665 > src/kicontheme.cpp 0996054 > > Diff: https://git.reviewboard.kde.org/r/127779/diff/ > > > Testing > --- > > > File Attachments > > > dadel1.png > > https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/0fc42425-947c-479e-9759-09c7a703a456__dadel1.png > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127779: use system colors for monochrome icons
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127779/#review95075 --- Ship it! Okay, let's try this in master and see if it's going to cause problems. - Sebastian Kügler On April 29, 2016, 9:25 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127779/ > --- > > (Updated April 29, 2016, 9:25 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kiconthemes > > > Description > --- > > Breeze uses stylesheet information to color its icons with some "named" > colors that change depending from the current system color scheme, this is > already used since some time in the icons used by the Plasma shell. > But QWidget applications have the same problem, when the user changes the > color scheme from breeze to breeze dark (or any color scheme), most of the > icons will be black on black. > This patch clones (a bit simplified and taking only the most "important" > colors) the logic used by Plasma::Svg to color icons with the stylesheet. > > even tough it's doing more things at icon generation, an application that > uses a lot of icons like Dolphin doesn't seem to have noticeable startup time > difference, even when the image cache is not present yet, so i hope is not an > unacceptable performance tradeoff (successive loads are unchanged as are from > the image cache). > > still some questions and things that can be optimized, like > > * an optional key in the theme metadata file to explicitly enable this, to > not have it running in themes that don't care? > > * can i use karchive in this framework?, so it would work with svgz icons as > well > > * right now to refresh icons at runtime it depends from a patch in the colors > kcm to emit iconchanges as well, alternatively kiconloader could watch for > kcolorscheme changes as well, but i don't think is strictly necessary > > > Diffs > - > > CMakeLists.txt 2e838e8 > autotests/coloredsvgicon.svg PRE-CREATION > autotests/kiconloader_unittest.cpp 0e47cc8 > autotests/resources.qrc a19c963 > src/CMakeLists.txt 0e30a35 > src/kiconloader.cpp 75ab482 > src/kicontheme.h 3190665 > src/kicontheme.cpp 0996054 > > Diff: https://git.reviewboard.kde.org/r/127779/diff/ > > > Testing > --- > > > File Attachments > > > dadel1.png > > https://git.reviewboard.kde.org/media/uploaded/files/2016/04/28/0fc42425-947c-479e-9759-09c7a703a456__dadel1.png > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127602: Add a test case for KWindowEffects::isEffectAvailable
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127602/#review94373 --- Ship it! Ship It! - Sebastian Kügler On April 7, 2016, 2:12 p.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127602/ > --- > > (Updated April 7, 2016, 2:12 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kwindowsystem > > > Description > --- > > Just verifies that reading the property on the root window works as > expected. > > > Diffs > - > > autotests/kwindoweffectstest.cpp 06a52a1e6e3d0fb4c967ff9c5295bb3a0381ee51 > > Diff: https://git.reviewboard.kde.org/r/127602/diff/ > > > Testing > --- > > > Thanks, > > Martin Gräßlin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127405: Make sure PlasmaQuick export file is properly found
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127405/#review93652 --- Ship it! Ship It! - Sebastian Kügler On March 17, 2016, 12:37 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127405/ > --- > > (Updated March 17, 2016, 12:37 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > Uses `"quotes"` rather than `` to include a neigbour header file. > > > Diffs > - > > src/plasmaquick/appletquickitem.h 99fcf1b > src/plasmaquick/configmodel.h ed2df2e > src/plasmaquick/configview.h 5ac9c26 > src/plasmaquick/dialog.h 63efa04 > src/plasmaquick/packageurlinterceptor.h 343e311 > src/plasmaquick/shellpluginloader.h bdc13be > src/plasmaquick/view.h b59271d > > Diff: https://git.reviewboard.kde.org/r/127405/diff/ > > > Testing > --- > > Aparently it was a problem on some systems. It was eventually fixed for me, > so I thought I was crazy. This should be the fix nevertheless (assuming > include directories are being exported properly). > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127418: Set text on QCheckbox widget rather than using a separate label
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127418/#review93685 --- Ship it! Ship It! - Sebastian Kügler On March 18, 2016, 1:51 a.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127418/ > --- > > (Updated March 18, 2016, 1:51 a.m.) > > > Review request for KDE Frameworks. > > > Bugs: 245580 > https://bugs.kde.org/show_bug.cgi?id=245580 > > > Repository: kio > > > Description > --- > > This fixes (amongst other focus bugs) the label having the right > enabled effect if the permissions are not editable. > > CCBUG: 245580 > > > Diffs > - > > src/widgets/kpropertiesdialog.cpp 59e189fb79e8033d8f8983b19aa914767dac52f6 > > Diff: https://git.reviewboard.kde.org/r/127418/diff/ > > > Testing > --- > > Opened kpropertiesdialog on a folder and a file > > Faked being a file system without ACL (by adding && false) into line 2128 so > that we see the full grid of options > > Opened kpropertiesdialog on a folder and a file > > > Thanks, > > David Edmundson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127345: Make it possible for an applet to offer a test object
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127345/#review93447 --- Ship it! Ship It! - Sebastian Kügler On March 12, 2016, 1:13 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127345/ > --- > > (Updated March 12, 2016, 1:13 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > In an attempt to make it possible for plasmoid to test themselves in the > different shells, provide API to exatract the object that will test the > plasmoid instance. > > > Diffs > - > > src/plasma/corona.cpp 5d17550 > src/plasma/private/packages.cpp a5ba81a > src/plasmaquick/appletquickitem.h 4f25f5d > src/plasmaquick/appletquickitem.cpp 9242e9e > src/plasmaquick/private/appletquickitem_p.h 9c24734 > > Diff: https://git.reviewboard.kde.org/r/127345/diff/ > > > Testing > --- > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127345: Make it possible for an applet to offer a test object
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127345/#review93442 --- Fix it, then Ship it! Like my other review: if we want this approach, the code is fine and good to go. src/plasma/private/packages.cpp (line 46) <https://git.reviewboard.kde.org/r/127345/#comment63705> I'd just go for test.qml or perhaps even autotest.qml - Sebastian Kügler On March 12, 2016, 12:29 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127345/ > --- > > (Updated March 12, 2016, 12:29 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > In an attempt to make it possible for plasmoid to test themselves in the > different shells, provide API to exatract the object that will test the > plasmoid instance. > > > Diffs > - > > src/plasma/private/packages.cpp a5ba81a > src/plasmaquick/appletquickitem.h 4f25f5d > src/plasmaquick/appletquickitem.cpp 9242e9e > src/plasmaquick/private/appletquickitem_p.h 9c24734 > > Diff: https://git.reviewboard.kde.org/r/127345/diff/ > > > Testing > --- > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127345: Make it possible for an applet to offer a test object
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127345/#review93419
---
I have a rough idea how you want it to work, but to really evaluate the patch,
I need to understand the bigger picture.
Some smallish issues pointed out inline.
src/plasmaquick/appletquickitem.h (line 31)
<https://git.reviewboard.kde.org/r/127345/#comment63681>
unrelated?
(IMO no need to make another patch for it, just checking.)
src/plasmaquick/appletquickitem.h (line 129)
<https://git.reviewboard.kde.org/r/127345/#comment63682>
teh mistake!
src/plasmaquick/appletquickitem.cpp (line 652)
<https://git.reviewboard.kde.org/r/127345/#comment63683>
{ } also for single lines
- Sebastian Kügler
On March 11, 2016, 8:26 p.m., Aleix Pol Gonzalez wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127345/
> ---
>
> (Updated March 11, 2016, 8:26 p.m.)
>
>
> Review request for KDE Frameworks and Plasma.
>
>
> Repository: plasma-framework
>
>
> Description
> ---
>
> In an attempt to make it possible for plasmoid to test themselves in the
> different shells, provide API to exatract the object that will test the
> plasmoid instance.
>
>
> Diffs
> -
>
> src/plasma/private/packages.cpp a5ba81a
> src/plasmaquick/appletquickitem.h 4f25f5d
> src/plasmaquick/appletquickitem.cpp 9242e9e
> src/plasmaquick/private/appletquickitem_p.h 9c24734
>
> Diff: https://git.reviewboard.kde.org/r/127345/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Aleix Pol Gonzalez
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127090: Clean KWalletQuery cmake file
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127090/#review92744 --- Ship it! Ship It! - Sebastian Kügler On Feb. 16, 2016, 3:19 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127090/ > --- > > (Updated Feb. 16, 2016, 3:19 p.m.) > > > Review request for KDE Frameworks and Valentin Rusu. > > > Repository: kwallet > > > Description > --- > > I saw it didn't build because I needed a build with KDocTools, ended up > cleaning a bit more. > > > Diffs > - > > src/runtime/kwallet-query/CMakeLists.txt 42b4687 > src/runtime/kwallet-query/src/main.cpp c19cb29 > > Diff: https://git.reviewboard.kde.org/r/127090/diff/ > > > Testing > --- > > Built with and without KDocTools. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126804: DragArea: Implement grabbing delegate item
> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote: > > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 395 > > <https://git.reviewboard.kde.org/r/126804/diff/1/?file=434496#file434496line395> > > > > DPI-dependent, could be shared with the above > > David Rosca wrote: > Multiply with `window()->devicePixelRatio()` ? Yup, that'll work. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126804/#review91400 --- On Jan. 18, 2016, 9 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126804/ > --- > > (Updated Jan. 18, 2016, 9 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Implement grabbing image of delegate with QQuickItem::grabToImage. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 > > Diff: https://git.reviewboard.kde.org/r/126804/diff/ > > > Testing > --- > > QQuickItem::grabToImage is async, not sure how safe is to delay the start of > drag. It seems to work without any issues though. > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126804: DragArea: Implement grabbing delegate item
> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote: > > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311 > > <https://git.reviewboard.kde.org/r/126804/diff/1/?file=434496#file434496line311> > > > > So this means that we either pass an empty image, or an outdated one? > > I'm not entirely following the code, but I think the image should be > > redrawn when the drag starts, as the data may change and we definitely want > > to show the currently dragged data. > > David Rosca wrote: > No, at this point, the drag should be started. But if we have delegate > (QQuickItem), we must first render the image before starting the drag. If the > delegate is null, we start the drag immediately and as image use either > delegateImage or guess it from mimeData (this code path is not changed). Ow right, delegateImage gets set by the the user? If so, the delegateImage should be preferred to our rendered grab, no? - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126804/#review91400 --- On Jan. 18, 2016, 9 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126804/ > --- > > (Updated Jan. 18, 2016, 9 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Implement grabbing image of delegate with QQuickItem::grabToImage. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 > > Diff: https://git.reviewboard.kde.org/r/126804/diff/ > > > Testing > --- > > QQuickItem::grabToImage is async, not sure how safe is to delay the start of > drag. It seems to work without any issues though. > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126804: DragArea: Implement grabbing delegate item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126804/#review91400 --- Ow nice, I recall working on this during the initial Plasma5 porting and suitably neglected it after failing to get it to work nicely. Seems now we have much more reasonable Qt API to make this work nicely. (Or perhaps you're just smarter than I am. :)) Couple of comments inline, but looks already pretty good. src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 251) <https://git.reviewboard.kde.org/r/126804/#comment62478> So this means that we either pass an empty image, or an outdated one? I'm not entirely following the code, but I think the image should be redrawn when the drag starts, as the data may change and we definitely want to show the currently dragged data. src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 303) <https://git.reviewboard.kde.org/r/126804/#comment62475> This should be DPI-dependent src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 316) <https://git.reviewboard.kde.org/r/126804/#comment62476> Q_FOREACH and const& src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 321) <https://git.reviewboard.kde.org/r/126804/#comment62477> DPI-dependent, could be shared with the above - Sebastian Kügler On Jan. 18, 2016, 9 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126804/ > --- > > (Updated Jan. 18, 2016, 9 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Implement grabbing image of delegate with QQuickItem::grabToImage. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 > > Diff: https://git.reviewboard.kde.org/r/126804/diff/ > > > Testing > --- > > QQuickItem::grabToImage is async, not sure how safe is to delay the start of > drag. It seems to work without any issues though. > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126804: DragArea: Implement grabbing delegate item
> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote: > > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 390 > > <https://git.reviewboard.kde.org/r/126804/diff/1/?file=434496#file434496line390> > > > > Q_FOREACH and const& > > David Rosca wrote: > But the value is not used here, so the plain `for` could be faster? (no > accessing the data in container). > > Originally, it was Q_FOREACH, but it looked really weird to me. But sure, > if you think it would be better, I'll change it back. Ah, right. Good point. > On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote: > > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311 > > <https://git.reviewboard.kde.org/r/126804/diff/1/?file=434496#file434496line311> > > > > So this means that we either pass an empty image, or an outdated one? > > I'm not entirely following the code, but I think the image should be > > redrawn when the drag starts, as the data may change and we definitely want > > to show the currently dragged data. > > David Rosca wrote: > No, at this point, the drag should be started. But if we have delegate > (QQuickItem), we must first render the image before starting the drag. If the > delegate is null, we start the drag immediately and as image use either > delegateImage or guess it from mimeData (this code path is not changed). > > Sebastian Kügler wrote: > Ow right, delegateImage gets set by the the user? If so, the > delegateImage should be preferred to our rendered grab, no? > > David Rosca wrote: > Yes, and also `delegate` is set by user. I think we should prefer > `delegate` as it provides the exact image as the item being grabbed. But then > again, it doesn't really matter (though it should be noted in docs) - user > can choose to just don't set `delegate`. Makes sense, I'll leave it to you which one would be preferred. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126804/#review91400 --- On Jan. 18, 2016, 9 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126804/ > --- > > (Updated Jan. 18, 2016, 9 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Implement grabbing image of delegate with QQuickItem::grabToImage. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 > > Diff: https://git.reviewboard.kde.org/r/126804/diff/ > > > Testing > --- > > QQuickItem::grabToImage is async, not sure how safe is to delay the start of > drag. It seems to work without any issues though. > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126804: DragArea: Implement grabbing delegate item
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126804/#review91417 --- Fix it, then Ship it! src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 329) <https://git.reviewboard.kde.org/r/126804/#comment62492> With many urls in the mimedata, this could produce a very wide pixmap. I suggest either limiting the width by eliding, or doign multiple rows. This can be done in a subsequent patch. For now, it'd be good enough IMO to break out of the loop after like 8 icons. - Sebastian Kügler On Jan. 21, 2016, 2:26 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126804/ > --- > > (Updated Jan. 21, 2016, 2:26 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Implement grabbing image of delegate with QQuickItem::grabToImage. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 > > Diff: https://git.reviewboard.kde.org/r/126804/diff/ > > > Testing > --- > > QQuickItem::grabToImage is async, not sure how safe is to delay the start of > drag. It seems to work without any issues though. > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126806: DragDropEvent: Add ignore() function
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126806/#review91340 --- Ship it! Ship It! - Sebastian Kügler On Jan. 18, 2016, 9:44 p.m., David Rosca wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126806/ > --- > > (Updated Jan. 18, 2016, 9:44 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > Add ignore() function to make it possible to reject events. > > > Diffs > - > > src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h 9e57531 > src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp 679d1ab > src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp b7aeeb7 > > Diff: https://git.reviewboard.kde.org/r/126806/diff/ > > > Testing > --- > > > Thanks, > > David Rosca > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126740: Add a script for optimizing svgs
> On Jan. 14, 2016, 10:27 a.m., Marco Martin wrote: > > does the svg stuff still workk? > > this script goes well together the others i have (only in plasma-framework > > atm) to remove the hardcoded colors inkscape likes to put even when an > > element is using the stylesheet and the one to replace hardcoded colors > > with classes of the stylesheet. > > > > > > as for compressing: i was actually thinking of uncompressing the svgs in > > plasma-framework :p (mainly because git) > > > > what needs to happen is probably the compression to svgz to be done at > > build time and have all simple svgs in the repository > > Aleix Pol Gonzalez wrote: > What do you mean with "because git"? Because we might have the exact same > problem with svgo then. I'm doing an educated guess here: Marco wants readable diffs to icons, so it's easier to spot if the coloring / stylesheets are still correct. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126740/#review91055 --- On Jan. 14, 2016, 3 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126740/ > --- > > (Updated Jan. 14, 2016, 3 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: breeze-icons > > > Description > --- > > Dirk's review (https://git.reviewboard.kde.org/r/126738/) gave me the idea > that right now we're serving right away the svg's from inkscape and there's > room for improvement, potentially. > > This patch just introduces a script that optimizes the svg's using `svgo`. > > More could be done, like using gzip files, we can look into that if anyone's > interested. In fact, we used to use svgz for the icons, I wonder why that > changed. > > This will change the files in-place rather than as a build step, which is > what I considered first. The process to run svgo on every file was about 30 > minutes to 1h on my system, so I doubt it's really desirable. > > A reduced file size is important because it will greatly reduce disk IO, > which is a bottle-neck we have. > > > Diffs > - > > optimize-svg.sh PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126740/diff/ > > > Testing > --- > > ``` > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 32M icons > 32M icons-dark/ > > #run the script > > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 17M icons > 17M icons-dark/ > ``` > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126740: Add a script for optimizing svgs
> On Jan. 14, 2016, 12:22 p.m., andreas kainz wrote: > > I have no problems to optimzie the svg files BUT my problem with the svg > > files is that they become more complex all the time last time the > > stylesheet, now optimization, > > > > I love scripts BUT it would be awesome for us to have this scripts before > > an distribution build there packages. In git keep it as simple as possible > > and when an distro will use the icons use therefore an script (check if the > > stylesheets fit, compress, make png files, ) > > Aleix Pol Gonzalez wrote: > That's also the reason why I did it like this. This script doesn't need > to be run by the icon designer himself, it can be ran by the release manager > before creating the tarballs (or anyone else for that matter). > > I don't think it's a problem having non-optimized svgs in git, but we > should provide users whatever is easier to execute when we release. > > Sebastian Kügler wrote: > If we run this only on releases, it means that typically no developer > uses the final result of the icons. (I run master all the time, it's usually > a pure coincidence if I run an actual release on my laptop. If the icons only > get compressed or optimized right before a release, that means that I've run > unoptimized svgs, different from what our users end up using, which may yield > different results.) > > Aleix Pol Gonzalez wrote: > Would you get something like scripty to run it? I've thought about it, I think it could work if it doesn't rewrite the whole file every time it runs. (But only fixes 'new problems', if introduced.) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126740/#review91064 --- On Jan. 14, 2016, 3 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126740/ > --- > > (Updated Jan. 14, 2016, 3 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: breeze-icons > > > Description > --- > > Dirk's review (https://git.reviewboard.kde.org/r/126738/) gave me the idea > that right now we're serving right away the svg's from inkscape and there's > room for improvement, potentially. > > This patch just introduces a script that optimizes the svg's using `svgo`. > > More could be done, like using gzip files, we can look into that if anyone's > interested. In fact, we used to use svgz for the icons, I wonder why that > changed. > > This will change the files in-place rather than as a build step, which is > what I considered first. The process to run svgo on every file was about 30 > minutes to 1h on my system, so I doubt it's really desirable. > > A reduced file size is important because it will greatly reduce disk IO, > which is a bottle-neck we have. > > > Diffs > - > > optimize-svg.sh PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126740/diff/ > > > Testing > --- > > ``` > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 32M icons > 32M icons-dark/ > > #run the script > > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 17M icons > 17M icons-dark/ > ``` > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126740: Add a script for optimizing svgs
> On Jan. 14, 2016, 12:22 p.m., andreas kainz wrote: > > I have no problems to optimzie the svg files BUT my problem with the svg > > files is that they become more complex all the time last time the > > stylesheet, now optimization, > > > > I love scripts BUT it would be awesome for us to have this scripts before > > an distribution build there packages. In git keep it as simple as possible > > and when an distro will use the icons use therefore an script (check if the > > stylesheets fit, compress, make png files, ) > > Aleix Pol Gonzalez wrote: > That's also the reason why I did it like this. This script doesn't need > to be run by the icon designer himself, it can be ran by the release manager > before creating the tarballs (or anyone else for that matter). > > I don't think it's a problem having non-optimized svgs in git, but we > should provide users whatever is easier to execute when we release. If we run this only on releases, it means that typically no developer uses the final result of the icons. (I run master all the time, it's usually a pure coincidence if I run an actual release on my laptop. If the icons only get compressed or optimized right before a release, that means that I've run unoptimized svgs, different from what our users end up using, which may yield different results.) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126740/#review91064 --- On Jan. 14, 2016, 3 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126740/ > --- > > (Updated Jan. 14, 2016, 3 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: breeze-icons > > > Description > --- > > Dirk's review (https://git.reviewboard.kde.org/r/126738/) gave me the idea > that right now we're serving right away the svg's from inkscape and there's > room for improvement, potentially. > > This patch just introduces a script that optimizes the svg's using `svgo`. > > More could be done, like using gzip files, we can look into that if anyone's > interested. In fact, we used to use svgz for the icons, I wonder why that > changed. > > This will change the files in-place rather than as a build step, which is > what I considered first. The process to run svgo on every file was about 30 > minutes to 1h on my system, so I doubt it's really desirable. > > A reduced file size is important because it will greatly reduce disk IO, > which is a bottle-neck we have. > > > Diffs > - > > optimize-svg.sh PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126740/diff/ > > > Testing > --- > > ``` > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 32M icons > 32M icons-dark/ > > #run the script > > kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/ > 17M icons > 17M icons-dark/ > ``` > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: KIOGui ?
On Wednesday, January 13, 2016 12:33:36 AM David Faure wrote: > The question is: this would only depend on KIOCore and QImage. Shall I put > it in KIOWidgets or shall I create a new KIOGui library, for QML apps to > avoid the QWidget dependency ? I'd use this in the angelfish (experimental) webbrowser, which is QtQuick- based and would like to avoid QtWidgets. +1 for KIOGui. -- sebas http://www.kde.org | http://vizZzion.org ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126672: Fix most of Clazy warnings in plasma-framework
> On Jan. 11, 2016, 12:38 p.m., Sebastian Kügler wrote:
> > autotests/coronatest.cpp, line 215
> > <https://git.reviewboard.kde.org/r/126672/diff/1/?file=429035#file429035line215>
> >
> > Do we really not want the C++11 for here? If we don't, I'd prefer
> > Q_FOREACH as replacement, the Q_ macros seem safer in the long run.
>
> Aleix Pol Gonzalez wrote:
> It also detaches.
>
> http://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/
but wouldn't with for (const Plasma::Containment *cont :
m_corona->containments()) {
... right? Would IMO be better, since it's explicit that way.
(Thanks for the other explanations, Aleix!)
- Sebastian
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126672/#review90890
---
On Jan. 8, 2016, 1:34 a.m., Sergey Popov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126672/
> ---
>
> (Updated Jan. 8, 2016, 1:34 a.m.)
>
>
> Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez.
>
>
> Repository: plasma-framework
>
>
> Description
> ---
>
> Fix almost all the Clazy warnings in plasma-framework.
>
> Related GCI-2015 task:
> [https://codein.withgoogle.com/tasks/6272625345036288/](https://codein.withgoogle.com/tasks/6272625345036288/)
>
>
> Diffs
> -
>
> autotests/coronatest.cpp 378a4b7
> autotests/dialogqmltest.cpp 618e64d
> autotests/fallbackpackagetest.cpp 91bc6e9
> autotests/packagestructuretest.cpp 67cdb4f
> autotests/pluginloadertest.cpp 868d5f8
> autotests/sortfiltermodeltest.cpp 6ee0e35
> autotests/storagetest.cpp 8a7dbd0
> src/declarativeimports/calendar/calendar.cpp 5515550
> src/declarativeimports/calendar/daysmodel.cpp 0f81b5a
> src/declarativeimports/core/corebindingsplugin.cpp adfdc29
> src/declarativeimports/core/datamodel.cpp 4449f30
> src/declarativeimports/core/datasource.cpp 4fe5dc5
> src/declarativeimports/core/tooltip.cpp a5e223b
> src/declarativeimports/core/tooltipdialog.cpp 6c3712e
> src/declarativeimports/core/units.cpp 1798727
> src/declarativeimports/core/windowthumbnail.cpp 21e655e
> src/declarativeimports/plasmacomponents/plasmacomponentsplugin.cpp 9e924b3
> src/declarativeimports/plasmacomponents/qmenuitem.cpp 287e9b3
>
> src/declarativeimports/plasmaextracomponents/plasmaextracomponentsplugin.cpp
> 1a25f06
> src/plasma/applet.cpp 4ce2d28
> src/plasma/containment.cpp 0beb196
> src/plasma/containmentactions.cpp f42807f
> src/plasma/corona.cpp bae9244
> src/plasma/datacontainer.cpp 396bc6d
> src/plasma/dataengine.cpp dd56807
> src/plasma/dataengineconsumer.cpp 7c9b5f9
> src/plasma/framesvg.cpp 81187dc
> src/plasma/package.cpp 09f36f3
> src/plasma/packagestructure.cpp 09ea698
> src/plasma/pluginloader.cpp d7c49f2
> src/plasma/private/applet_p.cpp 949c92e
> src/plasma/private/associatedapplicationmanager.cpp e1620e9
> src/plasma/private/componentinstaller.cpp 8fbef24
> src/plasma/private/containment_p.cpp 09ed2cd
> src/plasma/private/dataenginemanager.cpp 7862171
> src/plasma/private/packages.cpp 1edd55a
> src/plasma/private/service_p.h 8a48487
> src/plasma/private/storage.cpp bc6992e
> src/plasma/private/storagethread.cpp 91b490b
> src/plasma/private/svg_p.h 1d1000d
> src/plasma/private/theme_p.cpp 18419bb
> src/plasma/private/timetracker.cpp cdfe94b
> src/plasma/scripting/scriptengine.cpp b9f43fe
> src/plasma/service.cpp d603cf2
> src/plasma/svg.cpp 28abd00
> src/plasma/theme.cpp e9420e6
> src/plasmapkg/main.cpp 336b14e
> src/plasmapkg/plasmapkg.cpp 4626323
> src/plasmaquick/appletquickitem.cpp ec2ed24
> src/plasmaquick/configmodel.cpp df537c1
> src/plasmaquick/configview.cpp c4ab518
> src/plasmaquick/dialog.cpp 8f4ee57
> src/plasmaquick/dialogshadows.cpp db408ae
> src/plasmaquick/dialogshadows_p.h 7e17c12
> src/plasmaquick/packageurlinterceptor.cpp 5e349d2
> src/plasmaquick/private/packages.h aa08b11
> src/plasmaquick/private/packages.cpp 5275848
> src/scriptengines/qml/plasmoid/appletinterface.cpp 8e4979a
> src/scriptengines/qml/plasmoid/containmentinterface.cpp 31285a2
> src/scriptengines/qml/plasmoid/declarativeappletscript.cpp b15695b
> src/scriptengines/qml/plasmoid/wallpaperinterface.cpp 9ecd62b
>
Re: Review Request 126632: add a "Complementary" color scheme to kcolorscheme
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126632/#review90879 --- +1 from me, too. - Sebastian Kügler On Jan. 5, 2016, 11:16 a.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126632/ > --- > > (Updated Jan. 5, 2016, 11:16 a.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kconfigwidgets > > > Description > --- > > Since some releases, plasma-frameworks locally expands kcolorscheme to have a > new "Complementary" section: this is done for things like the lockscreen and > the logout ui, where the colors are flipped from the current theme. > A local extension there is kinda unfortunate as makes the color configs files > not completely compatible, and I think there is an use case for this in > normal applications as well (Gwenview switches to a dark palette when in > fullscreen mode for instance) > Since I want to make the default plasma theme follow the system color scheme, > I would need for it to support this "complementary" area as well. > > > Diffs > - > > src/kcolorscheme.h 22bc21b > src/kcolorscheme.cpp 427ffa4 > > Diff: https://git.reviewboard.kde.org/r/126632/diff/ > > > Testing > --- > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126660: Avoid finding the same package multiple times from different paths.
> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote: > > src/kpackage/packageloader.cpp, line 190 > > <https://git.reviewboard.kde.org/r/126660/diff/1/?file=428735#file428735line190> > > > > What does the category have to do with this? We should only be going by > > the id (the plugin name). > > Andreas Hartmetz wrote: > Depends on the scope in which an ID is guaranteed to be unique. If it's > guaranteed to be globally unique and independent of category, sure, I'm all > for not making it more complicated than necessary. > > Andreas Hartmetz wrote: > Given that the ID falls back to the metadata file name without extension, > and that you can have the same file name in different directories, it seems > like you *could* have a package of the same name in different categories. The > same goes for names supplied by the data in the metadata files. Some names > make sense in several places. > > Andreas Hartmetz wrote: > Sorry, the part about the filename doesn't apply. It comes from the > documentation of KPluginMetaData::pluginId() which doesn't apply here, given > that the files are all called metadata.desktop or metadata.json. Yes, and the directory name is actually the same as the plugin name -- you can rely on that. So just a stringlist would in fact already be enough, but as I said ... why not use lst to check if it's already in there. Performance, I guess? But yes, the only thing we need to look at is the plugin id. Nothing else matters. Well, we need to make sure that the most-local plugin is actually put in the list, otherwise you can't override with locally installed ones. > On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote: > > src/kpackage/packageloader.cpp, line 187 > > <https://git.reviewboard.kde.org/r/126660/diff/1/?file=428735#file428735line187> > > > > This doesn't actually add anything. A better name would IMO be: > > alreadyListed() or something along those lines. > > Andreas Hartmetz wrote: > It also adds it to the list, though. > > Andreas Hartmetz wrote: > The point is that it isn't const, and it also has a return value that > matters. The name -sort of- conveys both. That list is internal and shouldn't be reflected in the API, though... (We may entirely circumven this issue, see below.) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126660/#review90755 --- On Jan. 7, 2016, 3:21 p.m., Andreas Hartmetz wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126660/ > --- > > (Updated Jan. 7, 2016, 3:21 p.m.) > > > Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin. > > > Repository: kpackage > > > Description > --- > > That was a problem in a scenario such as mine, where I have distro > packages and self compiled packages. There were (from the UI) > indistinguishable, duplicate packages in the panel's add applets > UI, in the tray config dialog's "additional items" section, and > probably in other places, too. > Note that requiring to have no duplicate packages in XDG_DATA_DIRS > by removing entries from XGD_DATA_DIRS doesn't fly because /usr > cannot be removed for the non-KF5 things that it brings. > > > Diffs > - > > src/kpackage/packageloader.cpp 9f7dd48 > > Diff: https://git.reviewboard.kde.org/r/126660/diff/ > > > Testing > --- > > Checked for duplicate entries in "add applets" and tray config dialog, no > duplicates anymore. Also no duplicates anymore in KWin effects KCM. > > > Thanks, > > Andreas Hartmetz > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126618: Make CMake macro kcoreaddons_desktop_to_json() work on windows.
> On Jan. 3, 2016, 3:22 p.m., Alex Richardson wrote:
> > According to the documentation AppLocalDataLocation is the following:
> > "C:/Users//AppData/Local/", "C:/ProgramData/",
> > "", "/data"
> >
> > In which directory are the desktop files? Unfortunately I can't check as my
> > Windows machine broke a while back and I haven't compiled KF5 on Windows
> > since.
> >
> > Patch looks good to me otherwise as it will still check the same
> > directories change behaviour on Linux. Only minor issue is that the error
> > message is a little bit confusing now.
>
> Christoph Cullmann wrote:
> hi, same problem occurs on mac, too, i think a better fallback would be
> something install prefix relative, that would work on win + mac.
>
> Alex Richardson wrote:
> Would checking `${KDE_INSTALL_FULL_KSERVICETYPES5DIR} + path` first and
> then fall back to GenericDataLocation work on Windows and Mac? Or do we still
> need the AppDataLocation for runtime detection of the paths?
>
> Ralf Habacker wrote:
> From
> https://build.opensuse.org/build/home:rhabacker:branches:windows:mingw:win32:KF516/openSUSE_13.2/x86_64/mingw32-umbrello5/_log
> ...
> Installing:
> /home/abuild/rpmbuild/BUILDROOT/mingw32-umbrello5-2.18.99.6f6891a-2.25.x86_64/usr/i686-w64-mingw32/sys-root/mingw/share/applications/org.kde.umbrello.desktop
>
> Christoph Cullmann wrote:
> KDE_INSTALL_FULL_KSERVICETYPES5DIR should be good enough to have
> something working to be able to build KF5 on win/mac without patching. If the
> desktoptojson program should work even after packed into some
> installer/application bundle I guess we would need the appdata fallback, too.
> The question is: is that a use case needed. For me it is enough to be able to
> have it working in a devel setup.
>
> Alex Richardson wrote:
> This code can also be used at runtime through
> [KPluginMetaData::fromDesktopFile()](http://api.kde.org/frameworks-api/frameworks5-apidocs/kcoreaddons/html/classKPluginMetaData.html#ac6e87c110b4743ce1b16044c649838ba)
> although any users should probably be using JSON files directly.
>
> Kåre Särs wrote:
> The kpart.desktop file that KTextEditor was looking for is in
> /share/kservicetypes5/.
>
> I added AppLocalData to get the path which is normally
> "/bin/" for KDE aplications. I could also live with
> KDE_INSTALL_FULL_KSERVICETYPES5DIR, but generally I would try to avoid
> absolute paths hardcoded into binaries...
>
> I'll update the error printout.
Plasma packages use KPluginMetaData::fromDesktopFile(), this cannot be changed
easily, as almost all packages around (our own, and third party) are using
.desktop files. We're slowly transitioning, but it will take time. Removing
that would mean a lot of our stuff would break, and even more 3rd party
plasmoids, kwin scripts, etc..
- Sebastian
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126618/#review90496
---
On Jan. 3, 2016, 1:22 p.m., Kåre Särs wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126618/
> ---
>
> (Updated Jan. 3, 2016, 1:22 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kcoreaddons
>
>
> Description
> ---
>
> Without this patch kcoreaddons_desktop_to_json() will not find the destop
> file.
>
> On windows GenericDataLocation returns "C:/Users//AppData/Local" or
> "C:/ProgramData". That is not a path that contains any destip files ;)
>
> This patch adds AppLocalDataLocation to the seach if the previous search does
> not return a match.
>
> Another option would be to hardcode the absolute path to all places that uses
> kcoreaddons_desktop_to_json(), but that does not feel too nice...
>
> What other options do we have?
>
>
> Diffs
> -
>
> src/lib/plugin/desktopfileparser.cpp 1122af8
>
> Diff: https://git.reviewboard.kde.org/r/126618/diff/
>
>
> Testing
> ---
>
> KTextEditor compiles again on windows
>
>
> Thanks,
>
> Kåre Särs
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126618: Make CMake macro kcoreaddons_desktop_to_json() work on windows.
> On Jan. 3, 2016, 3:22 p.m., Alex Richardson wrote:
> > According to the documentation AppLocalDataLocation is the following:
> > "C:/Users//AppData/Local/", "C:/ProgramData/",
> > "", "/data"
> >
> > In which directory are the desktop files? Unfortunately I can't check as my
> > Windows machine broke a while back and I haven't compiled KF5 on Windows
> > since.
> >
> > Patch looks good to me otherwise as it will still check the same
> > directories change behaviour on Linux. Only minor issue is that the error
> > message is a little bit confusing now.
>
> Christoph Cullmann wrote:
> hi, same problem occurs on mac, too, i think a better fallback would be
> something install prefix relative, that would work on win + mac.
>
> Alex Richardson wrote:
> Would checking `${KDE_INSTALL_FULL_KSERVICETYPES5DIR} + path` first and
> then fall back to GenericDataLocation work on Windows and Mac? Or do we still
> need the AppDataLocation for runtime detection of the paths?
>
> Ralf Habacker wrote:
> From
> https://build.opensuse.org/build/home:rhabacker:branches:windows:mingw:win32:KF516/openSUSE_13.2/x86_64/mingw32-umbrello5/_log
> ...
> Installing:
> /home/abuild/rpmbuild/BUILDROOT/mingw32-umbrello5-2.18.99.6f6891a-2.25.x86_64/usr/i686-w64-mingw32/sys-root/mingw/share/applications/org.kde.umbrello.desktop
>
> Christoph Cullmann wrote:
> KDE_INSTALL_FULL_KSERVICETYPES5DIR should be good enough to have
> something working to be able to build KF5 on win/mac without patching. If the
> desktoptojson program should work even after packed into some
> installer/application bundle I guess we would need the appdata fallback, too.
> The question is: is that a use case needed. For me it is enough to be able to
> have it working in a devel setup.
>
> Alex Richardson wrote:
> This code can also be used at runtime through
> [KPluginMetaData::fromDesktopFile()](http://api.kde.org/frameworks-api/frameworks5-apidocs/kcoreaddons/html/classKPluginMetaData.html#ac6e87c110b4743ce1b16044c649838ba)
> although any users should probably be using JSON files directly.
>
> Kåre Särs wrote:
> The kpart.desktop file that KTextEditor was looking for is in
> /share/kservicetypes5/.
>
> I added AppLocalData to get the path which is normally
> "/bin/" for KDE aplications. I could also live with
> KDE_INSTALL_FULL_KSERVICETYPES5DIR, but generally I would try to avoid
> absolute paths hardcoded into binaries...
>
> I'll update the error printout.
>
> Sebastian Kügler wrote:
> Plasma packages use KPluginMetaData::fromDesktopFile(), this cannot be
> changed easily, as almost all packages around (our own, and third party) are
> using .desktop files. We're slowly transitioning, but it will take time.
> Removing that would mean a lot of our stuff would break, and even more 3rd
> party plasmoids, kwin scripts, etc..
>
> Christoph Cullmann wrote:
> We won't remove what works atm and I guess it will not matter that much
> if plasmoids/kwin stuff works "better" on win/mac or like they do atm.
It was just an example, the code in question is in the kpackage framework,
meaning it's not plasma or kwin specific, but holds for any app using it. (I
understand those don't work on Windows, due to this problem?)
- Sebastian
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126618/#review90496
---
On Jan. 3, 2016, 1:22 p.m., Kåre Särs wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126618/
> ---
>
> (Updated Jan. 3, 2016, 1:22 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kcoreaddons
>
>
> Description
> ---
>
> Without this patch kcoreaddons_desktop_to_json() will not find the destop
> file.
>
> On windows GenericDataLocation returns "C:/Users//AppData/Local" or
> "C:/ProgramData". That is not a path that contains any destip files ;)
>
> This patch adds AppLocalDataLocation to the seach if the previous search does
> not return a match.
>
> Another option would be to hardcode the absolute path to all places that uses
> kcoreaddons_desktop_to_json(), but that does not feel too nice...
>
> What other options do we have?
>
>
> Diffs
> -
>
> src/lib/plugin/desktopfileparser.cpp 1122af8
>
> Diff: https://git.reviewboard.kde.org/r/126618/diff/
>
>
> Testing
> ---
>
> KTextEditor compiles again on windows
>
>
> Thanks,
>
> Kåre Särs
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126494: Add cross compile support for desktoptojson.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126494/#review90546 --- Ship it! I can't see anything obviously wrong with it. (But also can't judge if it's obviously right.) Given the possible impact, I'd say go for it. - Sebastian Kügler On Dec. 23, 2015, 10:46 p.m., Ralf Habacker wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126494/ > --- > > (Updated Dec. 23, 2015, 10:46 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcoreaddons > > > Description > --- > > Add cross compile support for desktoptojson. > > > Diffs > - > > KF5CoreAddonsConfig.cmake.in dce3a4e65599b286d8fedbaa20235f5025f509e8 > > Diff: https://git.reviewboard.kde.org/r/126494/diff/ > > > Testing > --- > > Cross compiled package has been build at > https://build.opensuse.org/package/show/home:rhabacker:branches:windows:mingw:win32:KF516/mingw32-kcoreaddons > > > Thanks, > > Ralf Habacker > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126348: Make it possible to provide the metadata in json
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126348/#review90302 --- Ship it! Ship It! - Sebastian Kügler On Dec. 15, 2015, 3:24 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126348/ > --- > > (Updated Dec. 15, 2015, 3:24 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kpackage > > > Description > --- > > Due to KCoreAddons transition from using desktop files to json files, > KPackage ended in a weird state where desktop files were allowed to use > desktop files but not json. > > This patch makes sure that manifest.json files are also acceptable. > > > Diffs > - > > autotests/data/testjsonmetadatapackage/contents/ui/main.qml PRE-CREATION > autotests/data/testjsonmetadatapackage/metadata.json PRE-CREATION > autotests/querytest.cpp 0a2f11a > src/kpackage/package.cpp 8416054 > src/kpackage/packageloader.cpp 1e1e382 > src/kpackage/private/packagejobthread.cpp 444dacc > src/kpackage/private/packages.cpp 2f6bbcf > > Diff: https://git.reviewboard.kde.org/r/126348/diff/ > > > Testing > --- > > Tests still pass (except for > https://build.kde.org/job/kpackage%20master%20stable-kf5-qt5/37/PLATFORM=Linux,compiler=gcc/testReport/, > that is not passing in master). > Adds a test plugin with a json file in it. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126472: Don't always load the timetracker
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126472/#review89924 --- Ship it! Nice. I agree with Aleix that the env var would be good to have documented. (Is that even English?) But then, where? - Sebastian Kügler On Dec. 22, 2015, 2:14 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126472/ > --- > > (Updated Dec. 22, 2015, 2:14 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > Don't always load the timetracker > > It has a timer that wakes up every 2 seconds and drains my battery just > simply because I have debug builds. This make it on demand. > > Don't track containments twice. > Containments inherit from Applets which also have the same line > > > Diffs > - > > src/plasma/corona.cpp 316b225ecafcfa6e0b8b894cfea082046c296238 > src/plasma/private/applet_p.cpp 511b45119f6d91605f36c8b7073a0b0bb4560e5f > src/plasma/private/containment_p.cpp > 86d27c43f36b9e1741ab13f8153080fb6b92b321 > > Diff: https://git.reviewboard.kde.org/r/126472/diff/ > > > Testing > --- > > > Thanks, > > David Edmundson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126418: Add KWin/Decoration support to plasmapkg
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126418/#review89816 --- Ship it! Ship It! - Sebastian Kügler On Dec. 18, 2015, 6:30 p.m., Demitrius Belai wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126418/ > --- > > (Updated Dec. 18, 2015, 6:30 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > Hi, > > This is same #105677 but for plasmapkg2 > > > Diffs > - > > src/plasmapkg/plasmapkg.cpp 4626323cd5d1dc3a3147fe43102d901d615e0b66 > > Diff: https://git.reviewboard.kde.org/r/126418/diff/ > > > Testing > --- > > > Thanks, > > Demitrius Belai > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126369: [OS X] adaptation(s) to platform limitations (WIP)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126369/#review89603 --- Looks fine to me, but I'll let Martin decide, since he maintains knotifications. - Sebastian Kügler On Dec. 15, 2015, 7:44 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126369/ > --- > > (Updated Dec. 15, 2015, 7:44 p.m.) > > > Review request for KDE Software on Mac OS X and KDE Frameworks. > > > Repository: knotifications > > > Description > --- > > OS X has a number of limitations in features used by KNotifications, notably > concerning the status notifier item (aka system tray icon). > > This RR will likely evolve to address multiple limitations (at least also the > NeedsAttention state); at the moment it only proposes an emulation of > `QMenu::addSection`. > > `QMenu::addSection` works by adding a QAction with a "texted separator" at > the insertion location. Texted separators do not exist in menu items in the > OS X "global" menubar (they become regular separators), and Qt will not > provide a platform-specific implementation. Loss of the section title text is > maybe not always an issue, but I think it is in the system tray menu. I > therefore propose to emulate `QMenu::addSection` by replacing the texted > separator with an inactive (disabled) menu item that shows the text, followed > by a standard separator. Menus in the notification area are much less subject > to interface guidelines, so the presence of an item icon is acceptable and > IMO useful for the `titleAction`. > > Testing the NeedsAttention state with the tests/kstatusnotifieritemtest > application leads to disappearance of the menubar icon, i.e. the access to > the notifier menu becomes invisible rather than blinking (which is what I get > on Linux using the same packaging). Adding a few qDebug statements shows that > the `attentionIcon` is empty. > I'd appreciate a crash course how this feature is supposed to work, so I can > see if an OS X implementation might be feasible. > > > Diffs > - > > src/kstatusnotifieritem.cpp f9bf460 > > Diff: https://git.reviewboard.kde.org/r/126369/diff/ > > > Testing > --- > > On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.17.0 . > > > Thanks, > > René J.V. Bertin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126348: Make it possible to provide the metadata in json
> On Dec. 14, 2015, 9:50 p.m., Sebastian Kügler wrote: > > src/kpackage/private/packagejobthread.cpp, line 189 > > <https://git.reviewboard.kde.org/r/126348/diff/1/?file=422766#file422766line189> > > > > Why this change? > > Aleix Pol Gonzalez wrote: > because currently it's not a text file. I can commit separately if you > want. .desktop and .json are not text files? (Sorry, trying to not be dense here, but I don't understand...) - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126348/#review89499 --- On Dec. 15, 2015, 3:24 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126348/ > --- > > (Updated Dec. 15, 2015, 3:24 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kpackage > > > Description > --- > > Due to KCoreAddons transition from using desktop files to json files, > KPackage ended in a weird state where desktop files were allowed to use > desktop files but not json. > > This patch makes sure that manifest.json files are also acceptable. > > > Diffs > - > > autotests/data/testjsonmetadatapackage/contents/ui/main.qml PRE-CREATION > autotests/data/testjsonmetadatapackage/metadata.json PRE-CREATION > autotests/querytest.cpp 0a2f11a > src/kpackage/package.cpp 8416054 > src/kpackage/packageloader.cpp 1e1e382 > src/kpackage/private/packagejobthread.cpp 444dacc > src/kpackage/private/packages.cpp 2f6bbcf > > Diff: https://git.reviewboard.kde.org/r/126348/diff/ > > > Testing > --- > > Tests still pass (except for > https://build.kde.org/job/kpackage%20master%20stable-kf5-qt5/37/PLATFORM=Linux,compiler=gcc/testReport/, > that is not passing in master). > Adds a test plugin with a json file in it. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126366: Make KPluginMetaData constructible from a json path.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126366/#review89553 --- An autotest would be nice, and I think the ASSERT is too aggressive. src/lib/plugin/kpluginmetadata.cpp (line 68) <https://git.reviewboard.kde.org/r/126366/#comment61279> I don't like the assert here. It may be third party data, and we're crashing the application then? - Sebastian Kügler On Dec. 15, 2015, 3:04 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126366/ > --- > > (Updated Dec. 15, 2015, 3:04 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcoreaddons > > > Description > --- > > Loads the json file upon construction. Simplifies adoption of json metadata > files for KPackage. > > > Diffs > - > > src/lib/plugin/kpluginmetadata.h a91b94a > src/lib/plugin/kpluginmetadata.cpp 3674360 > > Diff: https://git.reviewboard.kde.org/r/126366/diff/ > > > Testing > --- > > Tests still pass. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126343: KPluginMetaData: Add fields used by KAboutData and introduce conversion function
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126343/#review89472 --- Looks good to me, though I wouldn't mind another reviewer. (If no one steps up within a week or so, please ship it.) src/lib/kaboutdata.cpp (line 823) <https://git.reviewboard.kde.org/r/126343/#comment61232> Unrelated, but doesn't match what we define as "KDE" anymore. - Sebastian Kügler On Dec. 14, 2015, 11:45 a.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126343/ > --- > > (Updated Dec. 14, 2015, 11:45 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kcoreaddons > > > Description > --- > > Add KPluginMetaData::translators() and KAboutPerson::fromJson() > > Plugins could have a list of translators in KDE4 through KAboutData so > this commit restores this for KF5. As a side effect of now having > authors() and translators() returning a list of KAboutPerson I also > added KAboutPerson::fromJSON() as publc function but I can make that > internal API instead if it's preferred. > > --- > > Add KPluginMetaData::copyrightText(), extraInformation() and > otherContributors() > > Now KPluginMetaData has well defined keys for all the information used > by KAboutData. This means we can easily create KAboutData objects from > a KPluginMetaData object e.g. for use in an KAboutApplicationDialog. > For example Okular uses this to display information about generator that > is being used for the current document. > > --- > > Add KAboutData::fromPluginMetaData(const KPluginMetaData ) > > This is useful e.g. to create an KAboutApplicationDialog for plugins > > > Diffs > - > > autotests/kpluginmetadatatest.cpp 25c8b8d5351eb5c5c784d528f9e90dfc8ea77982 > src/lib/kaboutdata.h 0dbd7a0ce7ec9220614c5f824a0f379ea5f45559 > src/lib/kaboutdata.cpp 5d9a55ea027fc1b8fb6e6394d72cf48f142bb664 > src/lib/plugin/kpluginmetadata.h a91b94ad404b6839aa3b8d13b8da19f407708c1b > src/lib/plugin/kpluginmetadata.cpp 36743600788693b6449d065f0448e3195445772e > > Diff: https://git.reviewboard.kde.org/r/126343/diff/ > > > Testing > --- > > Tests pass, used by KAboutApplicationDialog in okular: > https://git.reviewboard.kde.org/r/126193/ > > > Thanks, > > Alex Richardson > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126348: Make it possible to provide the metadata in json
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126348/#review89499 --- src/kpackage/package.cpp (line 447) <https://git.reviewboard.kde.org/r/126348/#comment61251> Please remove before committing src/kpackage/private/packagejobthread.cpp (line 138) <https://git.reviewboard.kde.org/r/126348/#comment61252> So if this is going into KPluginMetaData, as Alex suggests, this part can be simplified src/kpackage/private/packagejobthread.cpp (line 160) <https://git.reviewboard.kde.org/r/126348/#comment61253> Would it make sense to make this stringlist a static as to share the data? src/kpackage/private/packagejobthread.cpp (line 183) <https://git.reviewboard.kde.org/r/126348/#comment61254> Why this change? src/kpackage/private/packagejobthread.cpp (line 303) <https://git.reviewboard.kde.org/r/126348/#comment61255> Above, we're using QStringLiteral() for the desktop file names, please make it consistent in the code you change, and in new code. Right now, it's a bit of a hodge-podge of QL1S, QSL, and just plain chars. src/kpackage/private/packagejobthread.cpp (line 312) <https://git.reviewboard.kde.org/r/126348/#comment61256> Not sure we should assert here, not loading a package is fine, and I'd rather not generate a crash (had to fix too many of these). Below, we're actually checking for valid metadata. Can we make it a warning? src/kpackage/private/packagejobthread_p.h (line 59) <https://git.reviewboard.kde.org/r/126348/#comment61258> We should be able to get rid of this one (and the additionally include in its user). src/kpackage/private/packages.cpp (line 57) <https://git.reviewboard.kde.org/r/126348/#comment61257> Fix spelling of "Fuck" and make it a fatal error? ;) - Sebastian Kügler On Dec. 14, 2015, 5:11 p.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126348/ > --- > > (Updated Dec. 14, 2015, 5:11 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kpackage > > > Description > --- > > Due to KCoreAddons transition from using desktop files to json files, > KPackage ended in a weird state where desktop files were allowed to use > desktop files but not json. > > This patch makes sure that manifest.json files are also acceptable. > > > Diffs > - > > autotests/querytest.cpp 0a2f11a > src/kpackage/package.cpp 8416054 > src/kpackage/packageloader.cpp 1e1e382 > src/kpackage/private/packagejobthread.cpp 444dacc > src/kpackage/private/packagejobthread_p.h 1babf0b > src/kpackage/private/packages.cpp 2f6bbcf > > Diff: https://git.reviewboard.kde.org/r/126348/diff/ > > > Testing > --- > > Tests still pass (except for > https://build.kde.org/job/kpackage%20master%20stable-kf5-qt5/37/PLATFORM=Linux,compiler=gcc/testReport/, > that is not passing in master). > Adds a test plugin with a json file in it. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: system tray test?
On Friday, December 11, 2015 12:19:51 PM René J.V. Bertin wrote: > Does KF5 provide any classes above Qt's for putting up and controlling an > icon (with/out menu) in the "system tray"? If so, is there a simple test > app I can try? > > Reason I'm asking: kwalletmanager5 isn't showing a systray interface like > kwalletmanager(4) does. Not on OS X (where Qt5's systray does work), and on > Linux under a KDE4 desktop I'm not sure what I'm getting. Not sure what exactly you mean with "putting up and controlling an icon in the system tray", but we do have a status notifier test app, you can find it in: plasma-workspace/applets/systemtray/tests/statusnotifier There are also some test programs in frameworks/knotifications/tests, you may want to look at. Cheers, -- sebas http://www.kde.org | http://vizZzion.org ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126246: Add test for dynamically changing file definitions
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126246/#review89354 --- Ship it! Ship It! autotests/packagestructuretest.cpp (line 102) <https://git.reviewboard.kde.org/r/126246/#comment61112> ws (just because RB makes it totally red) autotests/packagestructuretest.cpp (line 108) <https://git.reviewboard.kde.org/r/126246/#comment61113> seems unnecessary? - Sebastian Kügler On Dec. 4, 2015, 6:23 p.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126246/ > --- > > (Updated Dec. 4, 2015, 6:23 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: kpackage > > > Description > --- > > this, referred to https://git.reviewboard.kde.org/r/126244/ tests that adding > or removing a file definition depending on the path (and whatever criteria, > like metadata contents) works. since is already done in several places has to > work correctly > > > Diffs > - > > autotests/packagestructuretest.h de2038e > autotests/packagestructuretest.cpp 4784bfd > > Diff: https://git.reviewboard.kde.org/r/126246/diff/ > > > Testing > --- > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126226: kdetemplate_add_app_templates installs previews
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126226/ --- Review request for KDE Frameworks and Marco Martin. Repository: extra-cmake-modules Description --- kapptemplate can't deal with templats without previews, so make sure we install the preview image. Without this patch, the installed templates show up broken in kapptemplate. With it, they work as expected. I'm assuming here that the preview has the baseName + .png, otherwise we'd have to read Icon field from the .kdevtemplate file, but that seems way too much hassle. The error message resulting from a wrong file name will show the expected filename, so it doesn't exactly hide the error. Diffs - kde-modules/KDETemplateMacro.cmake 796c3f1 Diff: https://git.reviewboard.kde.org/r/126226/diff/ Testing --- Installed templates/ from plasma-framework, this patch makes them work in kapptemplate. Thanks, Sebastian Kügler ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126119: remove unused platformstatus kded
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126119/#review88618 --- Ship it! Ship It! - Sebastian Kügler On Nov. 19, 2015, 4:14 p.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126119/ > --- > > (Updated Nov. 19, 2015, 4:14 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Bugs: 348840 > http://bugs.kde.org/show_bug.cgi?id=348840 > > > Repository: plasma-framework > > > Description > --- > > This kded was written at the beginning of the kf5 port cycle, it was supposed > to do the runtime shell switching for transformable devices, but then it > ended up completely implemented in plasmashell and not much tested anyways > due to the lack of drivers. > this got completely unused, remove it (and eventually add it again in future > if when there will be hardware support will be considered a viable approach, > but probably not) > > > Diffs > - > > src/CMakeLists.txt 0ecb348 > src/platformstatus/CMakeLists.txt 114d9eb > src/platformstatus/org.kde.platformstatus.xml d5c81bc > src/platformstatus/platformstatus.cpp d38635a > src/platformstatus/platformstatus.desktop 50e25a4 > src/platformstatus/platformstatus.h 6c113ab > > Diff: https://git.reviewboard.kde.org/r/126119/diff/ > > > Testing > --- > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126084: Small KDeclarative cleanup
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126084/#review88420 --- Ship it! Ship It! - Sebastian Kügler On Nov. 16, 2015, 11:53 a.m., Aleix Pol Gonzalez wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126084/ > --- > > (Updated Nov. 16, 2015, 11:53 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kdeclarative > > > Description > --- > > * Remove unread attribute. > * Remove unneeded include. > * Remove unneeded constructor. > > > Diffs > - > > src/kdeclarative/kdeclarative.cpp be795b2 > src/kdeclarative/private/kdeclarative_p.h 5bb0241 > src/kdeclarative/qmlobjectsharedengine.cpp 9a05e26 > > Diff: https://git.reviewboard.kde.org/r/126084/diff/ > > > Testing > --- > > Builds, tests pass. > > > Thanks, > > Aleix Pol Gonzalez > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126004: add symlink verification tech
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126004/#review88189 --- Ship it! Good stuff! - Sebastian Kügler On Nov. 9, 2015, 11:21 a.m., Harald Sitter wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126004/ > --- > > (Updated Nov. 9, 2015, 11:21 a.m.) > > > Review request for KDE Frameworks. > > > Repository: breeze-icons > > > Description > --- > > to ensure greatest possible compatibility icon names are often symlinked > to other names. unfortunately symlinks can easily break without anyone > noticing. the symlinktest is supposed to prevent this by iterating the > entire project tree's symlinks and checking if they are actually pointing > to something. > additionally symlinks must not point to out-of-tree files. even if valid. > > > Diffs > - > > COPYING.LIB PRE-CREATION > autotests/CMakeLists.txt PRE-CREATION > CMakeLists.txt 130b3346a6dc6dd86e65cc68bac6ccca19edf772 > autotests/symlinktest.cpp PRE-CREATION > autotests/testdata.h.cmake PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/126004/diff/ > > > Testing > --- > > ctest with both conditions true and false (master actually doesn't pass right > now :() > > > Thanks, > > Harald Sitter > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125976: add an update() method
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125976/#review88195 --- Ship it! Nice. Few issues with the wording in the comments, but generally looks good. src/kpackage/package.h (line 97) <https://git.reviewboard.kde.org/r/125976/#comment60482> /tmp may not be /tmp (definitely not on Windows) src/kpackage/package.h (line 98) <https://git.reviewboard.kde.org/r/125976/#comment60481> ...on the harddisk (though this should probably not say harddisk, but filesystem) src/kpackage/package.h (line 99) <https://git.reviewboard.kde.org/r/125976/#comment60483> -(from the hard disk) Nice. - Sebastian Kügler On Nov. 9, 2015, 2:40 p.m., Marco Martin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125976/ > --- > > (Updated Nov. 9, 2015, 2:40 p.m.) > > > Review request for KDE Frameworks, Plasma and Kai Uwe Broulik. > > > Repository: kpackage > > > Description > --- > > new job based update() function that compared to install() > if a package with the same pluginId is already installed, > removes the old one before installing the new one, if > and only if the version of the new one is more recent > > > Diffs > - > > autotests/plasmoidpackagetest.h f730dce > autotests/plasmoidpackagetest.cpp 567 > src/kpackage/CMakeLists.txt 3696f37 > src/kpackage/package.h 4ada8da > src/kpackage/package.cpp 539b21a > src/kpackage/packagestructure.h 9427b42 > src/kpackage/packagestructure.cpp 0070514 > src/kpackage/private/packagejob.cpp 0d2241b > src/kpackage/private/packagejob_p.h 267429f > src/kpackage/private/packagejobthread.cpp ca523b3 > src/kpackage/private/packagejobthread_p.h bf8a266 > src/kpackage/private/versionparser.cpp PRE-CREATION > src/kpackagetool/CMakeLists.txt 78e0fb0 > > Diff: https://git.reviewboard.kde.org/r/125976/diff/ > > > Testing > --- > > covered by autotests > > > Thanks, > > Marco Martin > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
