Re: Review Request: Corona::exportLayout
On 2010-09-26 20:22:05, Aaron Seigo wrote: trunk/KDE/kdelibs/plasma/corona.cpp, lines 359-362 http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line359 what does calling updateConstraints even do in this case? looking at the code in Applet, this just schedules a call to flushPendingConstraintsEvents with a timer. is it actually needed at all? iirc, it propogates the change to the applets... hmm, which wouldn't work if they were systemimmutable. so I ought to do it myself instead. On 2010-09-26 20:22:05, Aaron Seigo wrote: trunk/KDE/kdelibs/plasma/corona.cpp, line 340 http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340 this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group. unfortunately, Corona::importLayout() requires a KConfigBase which would make this assymetrical. iirc that bit of the API was written before i was aware of that KConfigGroup trick (which was actually undocumented until i found out about it :). perhaps we should add an importLayout that takes a KConfigGroup, mark the importLayout which takes a KConfigBase as deprecaton and have it call the new one. then we have a nice api with symmetry? so... void Corona::exportLayout(KConfigGroup *config, QListContainment* containments) and void Corona::importLayout(KConfigGroup *config) - Chani --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/#review7824 --- On 2010-09-25 16:51:30, Chani Armitage wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/ --- (Updated 2010-09-25 16:51:30) Review request for Plasma. Summary --- this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config. Activity::close() becomes a lot shorter by calling it, like this: m_corona-exportLayout(external, m_containments.values()); I feel a bit out of it today though, so tell me if I've missed anything obvious... Diffs - trunk/KDE/kdelibs/plasma/corona.cpp 1177115 trunk/KDE/kdelibs/plasma/corona.h 1177115 Diff: http://svn.reviewboard.kde.org/r/5451/diff Testing --- closing an activity while locked is perfectly safe now :) Thanks, Chani ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
On 2010-09-26 20:22:05, Aaron Seigo wrote:
trunk/KDE/kdelibs/plasma/corona.cpp, lines 359-362
http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line359
what does calling updateConstraints even do in this case? looking at
the code in Applet, this just schedules a call to
flushPendingConstraintsEvents with a timer. is it actually needed at all?
Chani Armitage wrote:
iirc, it propogates the change to the applets... hmm, which wouldn't work
if they were systemimmutable.
so I ought to do it myself instead.
i don't think updateConstraints does anything like that since here's the code
of that method:
// Don't start up a timer if we're just starting up
// flushPendingConstraints will be called by Corona
if (started !constraintsTimer.isActive() !(c
Plasma::StartupCompletedConstraint)) {
constraintsTimer.start(0, q);
}
if (c Plasma::StartupCompletedConstraint) {
started = true;
}
pendingConstraints |= c;
which means nothing happens until the event loop is hit again. setting the
immutability makes sense, just not the call to updateConstraints.
On 2010-09-26 20:22:05, Aaron Seigo wrote:
trunk/KDE/kdelibs/plasma/corona.cpp, line 340
http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340
this should probably take a KConfigGroup, since KConfigGroup can refer
to the top level item, e.g. the whole config file, using the magic
QString() name for the group.
unfortunately, Corona::importLayout() requires a KConfigBase which
would make this assymetrical. iirc that bit of the API was written before i
was aware of that KConfigGroup trick (which was actually undocumented until
i found out about it :).
perhaps we should add an importLayout that takes a KConfigGroup, mark
the importLayout which takes a KConfigBase as deprecaton and have it call
the new one.
then we have a nice api with symmetry?
Chani Armitage wrote:
so... void Corona::exportLayout(KConfigGroup *config, QListContainment*
containments)
and void Corona::importLayout(KConfigGroup *config)
KConfigGroup , but otherwise, yes...
- Aaron
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5451/#review7824
---
On 2010-09-25 16:51:30, Chani Armitage wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5451/
---
(Updated 2010-09-25 16:51:30)
Review request for Plasma.
Summary
---
this adds exportLayout to corona, which saves a group of containments to a
config file and deletes them from the main config.
Activity::close() becomes a lot shorter by calling it, like this:
m_corona-exportLayout(external, m_containments.values());
I feel a bit out of it today though, so tell me if I've missed anything
obvious...
Diffs
-
trunk/KDE/kdelibs/plasma/corona.cpp 1177115
trunk/KDE/kdelibs/plasma/corona.h 1177115
Diff: http://svn.reviewboard.kde.org/r/5451/diff
Testing
---
closing an activity while locked is perfectly safe now :)
Thanks,
Chani
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
On 2010-09-26 20:22:05, Aaron Seigo wrote:
trunk/KDE/kdelibs/plasma/corona.cpp, line 340
http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340
this should probably take a KConfigGroup, since KConfigGroup can refer
to the top level item, e.g. the whole config file, using the magic
QString() name for the group.
unfortunately, Corona::importLayout() requires a KConfigBase which
would make this assymetrical. iirc that bit of the API was written before i
was aware of that KConfigGroup trick (which was actually undocumented until
i found out about it :).
perhaps we should add an importLayout that takes a KConfigGroup, mark
the importLayout which takes a KConfigBase as deprecaton and have it call
the new one.
then we have a nice api with symmetry?
Chani Armitage wrote:
so... void Corona::exportLayout(KConfigGroup *config, QListContainment*
containments)
and void Corona::importLayout(KConfigGroup *config)
Aaron Seigo wrote:
KConfigGroup , but otherwise, yes...
I thought it was bad form to use without const...?
On 2010-09-26 20:22:05, Aaron Seigo wrote:
trunk/KDE/kdelibs/plasma/corona.cpp, lines 359-362
http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line359
what does calling updateConstraints even do in this case? looking at
the code in Applet, this just schedules a call to
flushPendingConstraintsEvents with a timer. is it actually needed at all?
Chani Armitage wrote:
iirc, it propogates the change to the applets... hmm, which wouldn't work
if they were systemimmutable.
so I ought to do it myself instead.
Aaron Seigo wrote:
i don't think updateConstraints does anything like that since here's the
code of that method:
// Don't start up a timer if we're just starting up
// flushPendingConstraints will be called by Corona
if (started !constraintsTimer.isActive() !(c
Plasma::StartupCompletedConstraint)) {
constraintsTimer.start(0, q);
}
if (c Plasma::StartupCompletedConstraint) {
started = true;
}
pendingConstraints |= c;
which means nothing happens until the event loop is hit again. setting
the immutability makes sense, just not the call to updateConstraints.
it's in ContainmentPrivate::containmentConstraintsEvent()
but anyways, moot point now :) I'll have it iterate through the applets and
access their dptr to force it to mutable.
- Chani
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5451/#review7824
---
On 2010-09-25 16:51:30, Chani Armitage wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5451/
---
(Updated 2010-09-25 16:51:30)
Review request for Plasma.
Summary
---
this adds exportLayout to corona, which saves a group of containments to a
config file and deletes them from the main config.
Activity::close() becomes a lot shorter by calling it, like this:
m_corona-exportLayout(external, m_containments.values());
I feel a bit out of it today though, so tell me if I've missed anything
obvious...
Diffs
-
trunk/KDE/kdelibs/plasma/corona.cpp 1177115
trunk/KDE/kdelibs/plasma/corona.h 1177115
Diff: http://svn.reviewboard.kde.org/r/5451/diff
Testing
---
closing an activity while locked is perfectly safe now :)
Thanks,
Chani
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
On 2010-09-26 20:22:05, Aaron Seigo wrote: trunk/KDE/kdelibs/plasma/corona.cpp, line 340 http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340 this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group. unfortunately, Corona::importLayout() requires a KConfigBase which would make this assymetrical. iirc that bit of the API was written before i was aware of that KConfigGroup trick (which was actually undocumented until i found out about it :). perhaps we should add an importLayout that takes a KConfigGroup, mark the importLayout which takes a KConfigBase as deprecaton and have it call the new one. then we have a nice api with symmetry? Chani Armitage wrote: so... void Corona::exportLayout(KConfigGroup *config, QListContainment* containments) and void Corona::importLayout(KConfigGroup *config) Aaron Seigo wrote: KConfigGroup , but otherwise, yes... Chani Armitage wrote: I thought it was bad form to use without const...? oh, for import is would be const KConfigGroup , duhh. :) but for export, KConfigGroup* is more correct, right? - Chani --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/#review7824 --- On 2010-09-25 16:51:30, Chani Armitage wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/ --- (Updated 2010-09-25 16:51:30) Review request for Plasma. Summary --- this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config. Activity::close() becomes a lot shorter by calling it, like this: m_corona-exportLayout(external, m_containments.values()); I feel a bit out of it today though, so tell me if I've missed anything obvious... Diffs - trunk/KDE/kdelibs/plasma/corona.cpp 1177115 trunk/KDE/kdelibs/plasma/corona.h 1177115 Diff: http://svn.reviewboard.kde.org/r/5451/diff Testing --- closing an activity while locked is perfectly safe now :) Thanks, Chani ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Use Plasma::ScrollWidget in the widget explorer
On 2010-09-24 10:43:08, Beat Wolf wrote: what is the status of this patch? Aurélien Gâteau wrote: I need to ping someone so that it gets reviewed. I like this idea because it cleans the interface, specially in the vertical orientation. The patch need to be updated due to recent changes in widgetexplorer.cpp, though. - Anselmo --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4676/#review7753 --- On 2010-08-04 21:02:11, Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4676/ --- (Updated 2010-08-04 21:02:11) Review request for Plasma. Summary --- Make AbstractIconList inherit from Plasma::ScrollWidget, has discussed on plasma-devel. The horizontal orientation behaved a bit strangely: AbstractIconList was becoming much larger than the screen width. I had to change the layout code to include the Close button inside FilteringWidget layout instead of creating another layout. Note: you need http://reviewboard.kde.org/r/4675/ to get proper scrollbar slider sizes. Diffs - trunk/KDE/kdebase/workspace/libs/plasmagenericshell/abstracticonlist.h 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/abstracticonlist.cpp 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletsfiltering.h 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletsfiltering.cpp 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletslist.h 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletslist.cpp 1147944 trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/widgetexplorer.cpp 1147944 Diff: http://svn.reviewboard.kde.org/r/4676/diff Testing --- Tested in both horizontal and vertical modes, with lists larger and smaller than the view. Thanks, Aurélien ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Do not use tree view for categories in the vertical widgets explorer
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5446/ --- Review request for Plasma. Summary --- The propose here is to remove the Tree View used in the widgets explorer in vertical orientation. In its place, use the same button of the horizontal orientation, what simplifies the code (removes a class and some checks) and improves the use of space as the widgets list can grow vertically. Better if combined with http://reviewboard.kde.org/r/4676/ as we also gain the space currently occupied by the arrow buttons and the close button in the bottom left corner. The screenshots attached show the current widget explorer with the categories tree view and the proposed one, using the categories button. Diffs - /trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletsfiltering.h 1179288 /trunk/KDE/kdebase/workspace/libs/plasmagenericshell/widgetsexplorer/appletsfiltering.cpp 1179288 Diff: http://svn.reviewboard.kde.org/r/5446/diff Testing --- Tested in both horizontal and vertical orientations. Screenshots --- current widgets explorer http://svn.reviewboard.kde.org/r/5446/s/511/ proposed, not using the tree view http://svn.reviewboard.kde.org/r/5446/s/512/ Thanks, Anselmo ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/ --- (Updated 2010-09-27 14:38:42.056313) Review request for Plasma. Changes --- updated API and fixed the immutability thing Summary --- this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config. Activity::close() becomes a lot shorter by calling it, like this: m_corona-exportLayout(external, m_containments.values()); I feel a bit out of it today though, so tell me if I've missed anything obvious... Diffs (updated) - trunk/KDE/kdelibs/plasma/corona.h 1179499 trunk/KDE/kdelibs/plasma/corona.cpp 1179499 Diff: http://svn.reviewboard.kde.org/r/5451/diff Testing --- closing an activity while locked is perfectly safe now :) Thanks, Chani ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Activity services merge
*** This is a cross-list-thread *** Hi all, I'm in the process of recreating the activity-related services and I'd like to merge the kded activities daemon and nepomuk activities service. Essentially, the current state is this: - kded activities daemon handles the data needed by workspaces (plasma, kwin) which can exist even without nepomuk, while when nepomuk is online, it acts like cache - nepomuk service which links resources (documents, apps...) to activities. Both will experience significant changes, mostly feature-wise. The reasons for the proposed merger into one service: - easier maintainability - less code duplication (both services need to know the list of activities, names etc.) - less d-bus communication (kded daemon needs to pass most things to the nepomuk service) Reasons why it was separated in the first place - kwin people didn't want to depend on nepomuk A: The merged service would continue to work w/o nepomuk running so, apart from the fact that the service will have to be linked against libnepomuk, nothing will change - kded module was kept as simple as possible to avoid crashing kded A: See below - nepomuk tracking of opened/closed/etc. documents should not depend on existence of activities A: This can be kept as well, it is only that both activities and tracking will live in one executable, which would be the case even w/o the merge - plasma people didn't complain about anything except of missing features in kded daemon whilch will be addressed anyway :) So, from my POV, the only remaining problem is crashing the kded if everything is put inside it. For this, there are two possible solutions: 1) Make an out-of-process kded module 2) Make an independent d-bus service which will start as soon as anybody tries to access some of its method (my favourite feature of d-bus) Thoughts? Complaints? If not, the merger will happen. Any ideas regarding the name of the service would be more than welcome. IIRC, Trueg had something against ActivityManager. For me, the alternative could be UsageTracker... but using Tracker in the name wouldn't be a good idea. Cheerio, Ivan -- Sanity is the trademark of a weak mind. -- Mark Harrold ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
On 2010-09-26 20:22:05, Aaron Seigo wrote: trunk/KDE/kdelibs/plasma/corona.cpp, line 340 http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340 this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group. unfortunately, Corona::importLayout() requires a KConfigBase which would make this assymetrical. iirc that bit of the API was written before i was aware of that KConfigGroup trick (which was actually undocumented until i found out about it :). perhaps we should add an importLayout that takes a KConfigGroup, mark the importLayout which takes a KConfigBase as deprecaton and have it call the new one. then we have a nice api with symmetry? Chani Armitage wrote: so... void Corona::exportLayout(KConfigGroup *config, QListContainment* containments) and void Corona::importLayout(KConfigGroup *config) Aaron Seigo wrote: KConfigGroup , but otherwise, yes... Chani Armitage wrote: I thought it was bad form to use without const...? Chani Armitage wrote: oh, for import is would be const KConfigGroup , duhh. :) but for export, KConfigGroup* is more correct, right? it's usually considered bad form, but in the case it's fine and prevents having to beware of null pointers in the lib code and keeps the API as symmetric as possible. it's also how we've handled KConfigGroup elsewhere in libplasma, and there are other areas of kde that do similarly. for KConfigGroup it simpler and sensible to pass it around by reference in these cases. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/#review7824 --- On 2010-09-27 14:38:42, Chani Armitage wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/ --- (Updated 2010-09-27 14:38:42) Review request for Plasma. Summary --- this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config. Activity::close() becomes a lot shorter by calling it, like this: m_corona-exportLayout(external, m_containments.values()); I feel a bit out of it today though, so tell me if I've missed anything obvious... Diffs - trunk/KDE/kdelibs/plasma/corona.h 1179499 trunk/KDE/kdelibs/plasma/corona.cpp 1179499 Diff: http://svn.reviewboard.kde.org/r/5451/diff Testing --- closing an activity while locked is perfectly safe now :) Thanks, Chani ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/#review7839 --- Ship it! a few small things, but this can go in with the details fixed :) trunk/KDE/kdelibs/plasma/corona.h http://svn.reviewboard.kde.org/r/5451/#comment8108 should be marked with KDE_DEPRECATED to generate compiler warnings. trunk/KDE/kdelibs/plasma/corona.h http://svn.reviewboard.kde.org/r/5451/#comment8109 KConfigGroup config, same as elsewhere in the libplasma API. trunk/KDE/kdelibs/plasma/corona.cpp http://svn.reviewboard.kde.org/r/5451/#comment8110 right now importLayout(const KConfigBase conf) checks to make sure that the KConfigGroup isValid(); this check should be moved to CoronaPrivate::importLayout so it can be shared with both importLayout methods. - Aaron On 2010-09-27 14:38:42, Chani Armitage wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/ --- (Updated 2010-09-27 14:38:42) Review request for Plasma. Summary --- this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config. Activity::close() becomes a lot shorter by calling it, like this: m_corona-exportLayout(external, m_containments.values()); I feel a bit out of it today though, so tell me if I've missed anything obvious... Diffs - trunk/KDE/kdelibs/plasma/corona.h 1179499 trunk/KDE/kdelibs/plasma/corona.cpp 1179499 Diff: http://svn.reviewboard.kde.org/r/5451/diff Testing --- closing an activity while locked is perfectly safe now :) Thanks, Chani ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group. the magic qstring thing means it takes two lines to call exportLayout. what's the benefit? ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Corona::exportLayout
On Monday, September 27, 2010, Chani wrote: this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group. the magic qstring thing means it takes two lines to call exportLayout. what's the benefit? a consistent API. -- Aaron J. Seigo humru othro a kohnu se GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 KDE core developer sponsored by Qt Development Frameworks signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: render SVGs with system colors
On Monday, September 27, 2010, Manuel Mommertz wrote: So would do you think of this? Should we try to get something like this in libplasma? Or is it to exotic? Maybe someone even has an idea how we can archive this in a clean way. Let me know. :) i think the idea is good, and the overhead should be acceptable for the svg's that use it. the file will only be read once, the expensive bit will be the xml parsing being done twice. the skel can be cached as well, created on first use and then kept around until the theme changes. have you done any timings to test how long the DOM parsing takes on svgs we have in the theme? (i'd also recommend putting the patch on reviewboard where it is easier to go through these kinds of things.) -- Aaron J. Seigo humru othro a kohnu se GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 KDE core developer sponsored by Qt Development Frameworks signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: render SVGs with system colors
On Monday 27 September 2010 23:09:16 Aaron J. Seigo wrote: ave you done any timings to test how long the DOM parsing takes on svgs we have in the theme? Not yet. But I have still some ideas to reduce the overhead. As soon as I feel fine with the code, I will do some timings. (i'd also recommend putting the patch on reviewboard where it is easier to go through these kinds of things.) See above. I put it there as soon as I have tested my ideas. Expect it next weekend. For now I am looking for comments on the idea itself and on the way this affects the SVGs. Manuel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
