Re: Review request: kde-runtime/kwalletd branch kwalletd-gpg
Noone taking a stab at this? Valentin do you know of someone you can annoy to ge the code reviewed? El Dissabte, 17 d'agost de 2013, a les 14:22:35, Valentin Rusu va escriure: > Hello, > > kwalletd now supports GPG encryption. The code is fully functional, at > least on my system, and I pushed it to the kwalletd-gpg branch of > kde-runtime. > See my blog post for more information: > http://www.rusu.info/wp/?p=248 > > I'd like now to ask you to review the code before merging it to the > master. And speaking about the master, I'd also like to know what would > be the good timing to do it, as 4.11 is a long-term support version. But > let's do things one after another and let's start with the feedback I'll > welcome from all of you. I'm also wondering if I should I use > reviewboard for this feature, knowing that's best to actually clone and > run the code in order to test it, and reviewboard only let us read the code? Well, if you give the people a way to see the diff that's awesome. How do i git diff your branch and master without seeing changes in master unrelated to your changes? Cheers, Albert > > Regards,
Re: KDE theme colors API for QML
El Dimecres, 14 d'agost de 2013, a les 23:49:23, Денис Купляков va escriure:
> Hey all!
Hi
> I want to implement API to access KDE theme colors from QML to use in
> games or other applications. For example it will allow us to implement
> KGamePopupItem substitution for QML.
>
> Now developer need to make own custom c++ writen class to access
> colors through Q_PROPERTY using. But we can't provide hundreds of
> props for every KDE color.
>
> So I decided to make universal API, that will use enums that are
> declared at KColorScheme class to access colors.
>
> I have started investigating what we can use for this:
>
> What we have:
> 1) Q_ENUMS macro that allows to use enumeration in MOS (metaobject
> system), can only be used at QObject derived class as other macros. We
> can also use it like this:
>
> Class A : QObject {
> Q_OBJECT
> public:
> enum TestEnum {
> FOO,
> BAR
> }
> }
>
> Class B: QObject {
> Q_OBJECT
> Q_ENUMS(A::TestEnum)
> PROPERTY(A::TestEnum someprop READ ... WRITE ... )
> }
>
> It allows to register enum from other QObject derived class at MOS,
> but if we get instance of B at QML we have no way to use enum. Also
> because of QObject we can't use it because KColorScheme is not QObject
> derived.
>
> OK, still if we write
>
> Class A : QObject {
> Q_OBJECT
> Q_ENUMS(TestEnum)
> public:
> enum TestEnum {
> FOO,
> BAR
> }
> }
>
> we can assign FOO and BAR only to properties of A or other class, we
> can't pass it as arguments (so we even can't do smth like
> console.log(A.FOO) from QML)
>
> 2) Q_INVOKABLE macro that allows to use function in MOS, can only be
> used at QObject derived class, at its arguments seems to be only MOS
> classes and primitive classes (for example: int, QString, QObject *)
>
> 3) Q_DECLARE_METATYPE macro it allows to register class at MOS and use
> as functions arguments. Here [1] and here [2] we see that people say
> it is applicable for enums.
>
> So there is a lot of limitations, and I have tried two approaches:
> FIRST) we are creating uncreatable (I usually say static class) (if
> smth go wrong can be creatable) ColorScheme (with
> qmlRegisterUncreatableType function) that will have functions like:
>
> Q_INVOKABLE QColor background(KColorScheme::
> ColorSet,
> KColorScheme::BackgroundRole, QPalette::ColorGroup)
>
> SECOND) we are creating ColorToken class that will have propeties like:
> colorSet, backgroundRole, colorGroup (it is arguments READ WRITE access)
> backgroundColor (it is what we need it has only READ access)
>
> Now I can't do normally nor first neither second, because I can't
> access KColorScheme enums as it is not QObject derived... And making
> copy of all of them will be hardly to maintain I think. Previously I
> was thinking that such thing will make a trick:
>
> class ColorScheme: public QObject, public KColorScheme {
> Q_OBEJECT
> Q_ENUMS()
> }
>
> but was disappointed looking at moc file, it don't see enums.
>
> Also even making KColorScheme Q_OBJECT derived don't gives ability to
> use enums as arguments for functions. So we have only SECOND approach.
>
> We simply doing all our stuff by directly changing
> KColorScheme class: adding Q_OBJECT and Q_ENUMS statements to it, than
> going by SECOND approach. But its also bad because of binary
> compatibility of libraries, as was discussed at another maillinglist
> [3].
Yes, changing the KColorScheme ABI is not possible for 4.x, you may want to
take this to kde-frameworks-devel for 5.x though.
Regarding your problem, have you tried something like
#include "kcolorscheme.h"
class ColorSchemeWrapper : public QObject {
Q_OBJECT
Q_ENUMS(ColorSet)
public:
enum ColorSet {
View = KColorSchme::View, // Keeps enum values in sync!
Window = KColorSchme::Window,
Button = KColorSchme::Button,
Selection = KColorSchme::Selection,
Tooltip = KColorSchme::Tooltip
};
};
?
That should help, it's a bit of a pain, but probably is your only option at
the moment.
Cheers,
Albert
>
> So I have found about Q_GADGET [4], can we use it to expose
> KColorScheme enums instead of Q_OBJECT. What are you think about all
> of this?
>
> [1] http://qt-project.org/forums/viewreply/39427/
> [2] http://qt-project.org/forums/viewthread/10308/
> [3] http://lists.kde.org/?t=13757362453&r=1&w=2
> [4]
> http://harmattan-dev.nokia.com/docs/platform-api-reference/xml/daily-docs/l
> ibqt4/qobject.html search for Q_GADGET
>
> Best regards,
> Denis Kuplyakov
Re: Review Request 112234: Save krunner dialog size when manually resized
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112234/ --- (Updated Aug. 25, 2013, 6:46 p.m.) Review request for kde-workspace. Changes --- Fix a case where the current size was stored as the default size when the dialog was collapsed Description --- This size was previously only saved on destruction, which not once has worked, whether I shut down the computer or log out normally. Now it will save (after 1 sec) after manually resizing the dialog. Diffs (updated) - krunner/interfaces/default/interface.h a0367a55043aa18229200ca191e2fab1ecb4a32f krunner/interfaces/default/interface.cpp 505e0aa6c02233fba0ff7ae9ce1133e8c7542104 Diff: http://git.reviewboard.kde.org/r/112234/diff/ Testing --- Thanks, Harald Hvaal
Re: Review Request 112241: Fix "Show Launcher when not running" option in taskbar widget
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112241/ --- (Updated Aug. 25, 2013, 2:44 p.m.) Status -- This change has been discarded. Review request for kde-workspace, Plasma and Eike Hein. Description --- Fix the crash in plasma-desktop caused by newer QML taskbar widget. Simple steps to reproduce this crash. 1) Pin any task/application to taskbar using "show launcher when not running" option. 2) Close application. 3) Desktop crashes. Reason : 1) In Tasks::itemGeometryChanged(int,int,int,int,int) line 300 it checks for three conditions, -> pointer to task is not null -> taskItem itself is not null -> scene is not null 2) This condition gets false when item is LauncherItem. In function later line 334 when calling iconRect.moveTopLeft(QPoint) function it gets crashed. Patch : This patch adds check in if condition to check if taskItem is TaskManager::LauncherItemType and return from function if this is launcher item. Diffs - plasma/desktop/applets/tasks/tasks.cpp c4aef4b Diff: http://git.reviewboard.kde.org/r/112241/diff/ Testing --- Testing compilation - check installation - check plasmoidviewer - check in panel - check independently - check Thanks, Bhushan Shah
Re: Review Request 112241: Fix "Show Launcher when not running" option in taskbar widget
> On Aug. 24, 2013, 3:17 p.m., Eike Hein wrote:
> > Hm, on the face of it, this patch doesn't really make sense ... launcher
> > items don't have an associated task, so the function should already return
> > early and the extra condition should be redundant. Unless there's a race
> > condition in the library somewhere ... but then it still wouldn't crash on
> > translating a QPoint.
> >
> > Thank you for the patch, but I really still have to find a way to reproduce
> > this bug before just applying this blindly - it might be treating a symptom
> > instead of addressing the root cause.
>
> Bhushan Shah wrote:
> Are you sure that this happens on 32 bit only?
>
> Eike Hein wrote:
> No, I'm not - except I can't reproduce it on any of my 64 bit machines,
> and I have to assume if it were a widespread bug, the number of reports we'd
> be getting would be much, much higher. Note that we didn't even get any
> reports through any of the pre-releases about this, AFAIR.
>
> I'm typing this from a cellphone right now, but I hope when I get home
> tonight I'll finally get around to setting up a 32 bit VM, and I'm hoping
> it'll crash there so I can throw all the gdb/valgrind/asan at it we got :).
>
> Thomas Lübking wrote:
> What bug and is "WId" involved?
>
> Bhushan Shah wrote:
> https://bugs.kde.org/show_bug.cgi?id=322283
>
> Hrvoje Senjan wrote:
> @Thomas
> bug#322283
>
> Thomas Lübking wrote:
> Thanks.
>
> From a brief review of libtaskmanager, TaskGroup::getMemberById(int id)
> returns AbstractGroupableItem* which could be (reimplemented)
> - TaskGroup*
> - LauncherItem*
> - TaskItem*
> but is happily static_cast'ed to TaskItem* and then accessed.
>
> The fact(?) that itemType returns LauncherItemType indicates that there
> can very well be different returns and then you're accessing random memory
> portions - it does absolutely not matter that the function pointer for
> ::task() points into garbage when the item is actually a Launcher - the
> garbage is still rather not null, there's no guarantee that this basic deref
> somehow would crash or fail (virtual ::task() needed to be moved to
> AbstractGroupableItem then)
>
> To know whether or why this has different implications on different
> architectures:
> Valgrind will tell you.
>
> For the moment
> - either ::getMemberById() is supposed to return different types than
> TaskItem here, then static_cast needs to be qobject_cast or dynamic_cast
> (pending on linkage)
> - or and ::getMemberById() that is *not* TaskItem must be considered a
> bug, then i'd start by adding an
> Q_ASSERT(!static_cast(taskItem->itemType()
> == TaskManager::TaskItemType))
>
>
>
> I don't really know, but as this thing seems to manage all kinds of
> items, but only updating rects for TaskItem's (and actually *every* grouped
> TaskItem for a TaskGroupType return ... so we don't get more bugs on "windows
> don't minimize to proper icon ;-P") makes sense, the correct solution is
> probably indeed:
>
> AbstractGroupableItem *item = rootGroup()->getMemberById(id);
>
> if (item->itemType() == TaskManager::LauncherItemType)
>return; // launchers have no windows, we just cause X11 errors and
> with a little luck a stupid gobject abort
> if (item->itemType() == TaskManager::TaskGroupType) {
>// build a WId list of all grouped Items
>...
> } else //if (item->itemType() == TaskManager::TaskItemType) {
>tasks << static_cast(taskItem)->task();
> }
>
> // search parrent and other juggling to figure the proper rectangle
> ...
Thomas: Building the list of group child windows is already done (the traversal
happens on the QML side).
- Eike
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112241/#review38481
---
On Aug. 24, 2013, 2 p.m., Bhushan Shah wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112241/
> ---
>
> (Updated Aug. 24, 2013, 2 p.m.)
>
>
> Review request for kde-workspace, Plasma and Eike Hein.
>
>
> Description
> ---
>
> Fix the crash in plasma-desktop caused by newer QML taskbar widget.
>
> Simple steps to reproduce this crash.
>
> 1) Pin any task/application to taskbar using "show launcher when not running"
> option.
> 2) Close application.
> 3) Desktop crashes.
>
> Reason :
>
> 1) In Tasks::itemGeometryChanged(int,int,int,int,int) line 300 it checks for
> three conditions,
>
> -> pointer to task is not null
> -> taskItem itself is not null
> -> scene is not null
>
> 2) This condition gets false when
Re: KF5 kwallet, kwalletd and wallet manager questions
On Sunday, 2013-08-25, Valentin Rusu wrote: > On 08/24/2013 02:32 PM, Kevin Krammer wrote: > > On Saturday, 2013-08-24, Albert Astals Cid wrote: > >> El Dissabte, 24 d'agost de 2013, a les 12:31:20, Valentin Rusu va escriure: > >>> * AFAIK, frameworks should be independent and self-contained. kwallet > >>> depends on kde-runtime/kwalletd. This component should be detached from > >>> kde-runtime sources and attached inside kwallet/src/kwalletd, > >>> preserving history if possible. I can handle this task if that's OK > >>> for you (Aron, David, Kevin?) > >> > >> This makes sense and AFAIK is what is intended with frameworks, i.e. if > >> a lib needs a binary, that binary and the lib should be shipped > >> together > > > > One thing that could be put into consideration is whether the library/API > > would work with any SecretService implementation or require kwalletd > > specifically. > > The kwallet API is only implemented by kwalletd AFAIK. So for this API's > cas, we should provide kwalletd along with it. > > The new secret service API, on the other hand, is likely to have several > implementation. In that cas, the daemon should not be systematically > tied-in. Ah, I see. I thought that the KDE client API for secret service would currently be part of that framework, but of course if it is separate than that is even better. The LXDE folks are looking into secure storage and currently seem to consider gnome-keyring as the daemon, so having a ready Qt API without runtime dependencies would certainly be of interest to them. > I think that future versions of the kwallet framework will have > configure options, to let packagers/users choose what to install. Right, makes sense. I was mostly concerned about the Secret Service part which I mistakingly thought was part of the kwallet framework dicussed here. Cheers, Kevin -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring signature.asc Description: This is a digitally signed message part.
Re: KF5 kwallet, kwalletd and wallet manager questions
On 08/24/2013 01:43 PM, Albert Astals Cid wrote: > El Dissabte, 24 d'agost de 2013, a les 12:31:20, Valentin Rusu va escriure: >> >> * Another piece of software related to kwallet is KWalletManager from >> kdeutils. I think that component should also be detached from kdeutils >> and attached under kwallet/src/manager. I can also handle this task if >> that's OK (Aron, David, Kevin but also Raphael?) > > Not sure this bundling makes sense. KWalletManager is a tool that if i am a > developer for Windows that wants to use the kwallet framework I don't need. > Actually it's a utility a regular user should probably never need unless you > use it to manually store stuff (I do) or you want to inspect what apps store > (I do). I also use the KWalletManager on windows as a secret storing tool, at my workplace. The other KDE components I use at work do not seem to use the kwalletd. Chrome for Windows do not detect kwallet as it does under Linux. So, AFAICT, Windows users are interested by using KWalletManager, and this requires kwalletd, which is packaged separately. I think that even for this case we should bundle these components together. That would also help other contributors better understand kwallet's structure and allow them contribute more easily. > > Of course I don't have much of a decision power on this but wanted to express > my concerns ;-) > > Cheers and congratulations for your new maintainership, > Albert > Thanks signature.asc Description: OpenPGP digital signature
Re: KF5 kwallet, kwalletd and wallet manager questions
On 08/24/2013 02:32 PM, Kevin Krammer wrote: > On Saturday, 2013-08-24, Albert Astals Cid wrote: >> El Dissabte, 24 d'agost de 2013, a les 12:31:20, Valentin Rusu va escriure: >>> >>> * AFAIK, frameworks should be independent and self-contained. kwallet >>> depends on kde-runtime/kwalletd. This component should be detached from >>> kde-runtime sources and attached inside kwallet/src/kwalletd, preserving >>> history if possible. I can handle this task if that's OK for you (Aron, >>> David, Kevin?) >> >> This makes sense and AFAIK is what is intended with frameworks, i.e. if a >> lib needs a binary, that binary and the lib should be shipped together > > One thing that could be put into consideration is whether the library/API > would work with any SecretService implementation or require kwalletd > specifically. The kwallet API is only implemented by kwalletd AFAIK. So for this API's cas, we should provide kwalletd along with it. The new secret service API, on the other hand, is likely to have several implementation. In that cas, the daemon should not be systematically tied-in. I think that future versions of the kwallet framework will have configure options, to let packagers/users choose what to install. signature.asc Description: OpenPGP digital signature
Review Request 112260: Enable Dict DataEngine on KF5
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112260/ --- Review request for kde-workspace. Description --- Enable dict DataEngine on KF5. Diffs - plasma/generic/dataengines/CMakeLists.txt a20ddae plasma/generic/dataengines/dict/CMakeLists.txt 2cab7c2 plasma/generic/dataengines/dict/dictengine.h 24dc265 plasma/generic/dataengines/dict/dictengine.cpp 127b0e0 Diff: http://git.reviewboard.kde.org/r/112260/diff/ Testing --- Compiles - check Installs - check Engine Explorer - check Functions - check Thanks, Bhushan Shah
Review Request 112258: ksysguard process list: better keyboard navigation
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112258/ --- Review request for kde-workspace. Description --- ksysguard has good keyboard handling, but it can be better. For example, it has annoyed me that I cannot use the del/shift-del shortcuts or get the context menu unless I explicitly tab my way down to the treeview (even though pressing up/down with the text field focused gives the false impression that the rows have focus). This diff makes this keyboard navigation more efficient. While the comments in the code should describe the behavior well enough, here they are listed: * Search field ** Pressing enter moves to first item in treeview and opens context menu ** Pressing down/pgdown will move actual focus to the treeview ** Focusing the text field clears the treeview selection. This emphasises that only one visual element is focused at a time, as well as that you can not interact with the items in the view until they have been selected. * Treeview ** Pressing up when on the first entry will move focus to the text field ** If you start typing, the focus will immediately be moved to the text field ** When focus is received, select first row in the treeview Diffs - libs/ksysguard/processui/ksysguardprocesslist.cpp ed2c1ff4e93041e4b1911e2643bfda6888d171bd Diff: http://git.reviewboard.kde.org/r/112258/diff/ Testing --- Thanks, Harald Hvaal
