[kde-workspace/KDE/4.11] plasma/desktop/shell/scripting: implement min/max sizes for panel length in the scripting
Git commit cc843f06f670fe2d06b1f7f84a0a3fc1a3d575ed by Aaron Seigo.
Committed on 11/11/2013 at 08:56.
Pushed by aseigo into branch 'KDE/4.11'.
implement min/max sizes for panel length in the scripting
should be forward ported to plasma2's panel system
CCMAIL:vercetti.m...@gmail.com
CCMAIL:plasma-devel@kde.org
M +80 -10 plasma/desktop/shell/scripting/panel.cpp
M +9-0plasma/desktop/shell/scripting/panel.h
http://commits.kde.org/kde-workspace/cc843f06f670fe2d06b1f7f84a0a3fc1a3d575ed
diff --git a/plasma/desktop/shell/scripting/panel.cpp
b/plasma/desktop/shell/scripting/panel.cpp
index 925af7a..e48b9b5 100644
--- a/plasma/desktop/shell/scripting/panel.cpp
+++ b/plasma/desktop/shell/scripting/panel.cpp
@@ -225,33 +225,103 @@ int Panel::length() const
}
}
-void Panel::setLength(int pixels)
+void Panel::setLength(int minPixels, int maxPixels)
{
Plasma::Containment *c = containment();
-if (pixels 0 || !c) {
+if ((minPixels 0 maxPixels 0) || !c) {
return;
}
PanelView *v = panel();
if (v) {
+if (minPixels 0) {
+minPixels = minLength();
+}
+
+if (maxPixels 0) {
+maxPixels = maxLength();
+}
+
+int pixels = 0;
+pixels = minPixels;
+
+if (minPixels maxPixels) {
+maxPixels = minPixels;
+}
+
+pixels = qBound(minPixels, pixels, maxPixels);
+
QRectF screen = c-corona()-screenGeometry(v-screen());
-QSizeF s = c-size();
+QSizeF size = c-size();
+QSizeF minSize = c-minimumSize();
+QSizeF maxSize = c-maximumSize();
if (c-formFactor() == Plasma::Vertical) {
-if (pixels screen.height() - v-offset()) {
+if (minPixels screen.height() - v-offset()) {
return;
}
-s.setHeight(pixels);
-} else if (pixels screen.width() - v-offset()) {
+size.setHeight(pixels);
+minSize.setHeight(minPixels);
+maxSize.setHeight(maxPixels);
+} else if (minPixels screen.width() - v-offset()) {
return;
} else {
-s.setWidth(pixels);
+size.setWidth(pixels);
+minSize.setWidth(minPixels);
+maxSize.setWidth(maxPixels);
}
-c-resize(s);
-c-setMinimumSize(s);
-c-setMaximumSize(s);
+//kDebug() sizes: minSize size maxSize;
+c-setMinimumSize(0, 0);
+c-setMaximumSize(QWIDGETSIZE_MAX, QWIDGETSIZE_MAX);
+c-resize(size);
+c-setMinimumSize(minSize);
+c-setMaximumSize(maxSize);
+v-pinchContainmentToCurrentScreen();
+}
+}
+
+void Panel::setLength(int pixels)
+{
+setLength(pixels, pixels);
+}
+
+int Panel::minLength() const
+{
+Plasma::Containment *c = containment();
+if (!c) {
+return 0;
+}
+
+if (c-formFactor() == Plasma::Vertical) {
+return c-minimumHeight();
+} else {
+return c-minimumWidth();
+}
+}
+
+void Panel::setMinLength(int pixels)
+{
+setLength(pixels, -1);
+}
+
+int Panel::maxLength() const
+{
+Plasma::Containment *c = containment();
+if (!c) {
+return 0;
}
+
+if (c-formFactor() == Plasma::Vertical) {
+return c-maximumHeight();
+} else {
+return c-maximumWidth();
+}
+}
+
+void Panel::setMaxLength(int pixels)
+{
+setLength(-1, pixels);
}
int Panel::height() const
diff --git a/plasma/desktop/shell/scripting/panel.h
b/plasma/desktop/shell/scripting/panel.h
index 2ef0fb8..8e3271d 100644
--- a/plasma/desktop/shell/scripting/panel.h
+++ b/plasma/desktop/shell/scripting/panel.h
@@ -51,6 +51,8 @@ class Panel : public Containment
// panel properties
Q_PROPERTY(QString alignment READ alignment WRITE setAlignment)
Q_PROPERTY(int offset READ offset WRITE setOffset)
+Q_PROPERTY(int minLength READ minLength WRITE setMinLength)
+Q_PROPERTY(int maxLength READ maxLength WRITE setMaxLength)
Q_PROPERTY(int length READ length WRITE setLength)
Q_PROPERTY(int height READ height WRITE setHeight)
Q_PROPERTY(QString hiding READ hiding WRITE setHiding)
@@ -68,6 +70,12 @@ public:
int offset() const;
void setOffset(int pixels);
+int minLength() const;
+void setMinLength(int pixels);
+
+int maxLength() const;
+void setMaxLength(int pixels);
+
int length() const;
void setLength(int pixels);
@@ -87,6 +95,7 @@ public Q_SLOTS:
void reloadConfig() { Applet::reloadConfig(); }
private:
+void setLength(int minPixels, int maxPixels);
PanelView *panel() const;
};
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[kde-workspace] /: block until the lock windows are shown; prevents sleeping before locking
Git commit 62c2c398611b2e6ef6e1e38ed79bc9540fc02ec9 by Aaron Seigo.
Committed on 14/03/2013 at 12:16.
Pushed by aseigo into branch 'master'.
block until the lock windows are shown; prevents sleeping before locking
accomplished by:
* synchronously calling desktopResized in the screen locker app
(this causes the locker windows to be created)
* writing to stdout, and listening for readyRead on the ksmserver side
this should probably go into the stable branch as well, but realy needs
broad testing first given that it touches ksmserver, security, sleep, etc.
CCMAIL:plasma-devel@kde.org
M +0-1ksmserver/screenlocker/greeter/greeterapp.cpp
M +3-1ksmserver/screenlocker/greeter/greeterapp.h
M +8-0ksmserver/screenlocker/greeter/main.cpp
M +4-1ksmserver/screenlocker/ksldapp.cpp
M +7-0plasma/screensaver/shell/main.cpp
http://commits.kde.org/kde-workspace/62c2c398611b2e6ef6e1e38ed79bc9540fc02ec9
diff --git a/ksmserver/screenlocker/greeter/greeterapp.cpp
b/ksmserver/screenlocker/greeter/greeterapp.cpp
index b70c9d6..e54c216 100644
--- a/ksmserver/screenlocker/greeter/greeterapp.cpp
+++ b/ksmserver/screenlocker/greeter/greeterapp.cpp
@@ -71,7 +71,6 @@ UnlockApp::UnlockApp()
initialize();
connect(desktop(), SIGNAL(resized(int)), SLOT(desktopResized()));
connect(desktop(), SIGNAL(screenCountChanged(int)),
SLOT(desktopResized()));
-QMetaObject::invokeMethod(this, desktopResized, Qt::QueuedConnection);
}
UnlockApp::~UnlockApp()
diff --git a/ksmserver/screenlocker/greeter/greeterapp.h
b/ksmserver/screenlocker/greeter/greeterapp.h
index f332bfc..951b1e3 100644
--- a/ksmserver/screenlocker/greeter/greeterapp.h
+++ b/ksmserver/screenlocker/greeter/greeterapp.h
@@ -44,12 +44,14 @@ public:
void setTesting(bool enable);
+public Q_SLOTS:
+void desktopResized();
+
protected:
virtual bool eventFilter(QObject *obj, QEvent *event);
private Q_SLOTS:
void viewStatusChanged(const QDeclarativeView::Status status);
-void desktopResized();
void resetRequestIgnore();
void suspendToRam();
void suspendToDisk();
diff --git a/ksmserver/screenlocker/greeter/main.cpp
b/ksmserver/screenlocker/greeter/main.cpp
index 180d64d..3d4de27 100644
--- a/ksmserver/screenlocker/greeter/main.cpp
+++ b/ksmserver/screenlocker/greeter/main.cpp
@@ -20,6 +20,9 @@ along with this program. If not, see
http://www.gnu.org/licenses/.
#include KDE/KCmdLineArgs
#include KDE/KLocale
#include KDE/KGlobal
+#include QDateTime
+
+#include iostream
#include greeterapp.h
@@ -63,5 +66,10 @@ int main(int argc, char* argv[])
app.setTesting(true);
}
args-clear();
+app.desktopResized();
+
+// This allow ksmserver to know when the applicaion has actually finished
setting itself up.
+// Crucial for blocking until it is ready, ensuring locking happens before
sleep, e.g.
+std::cout Locked at QDateTime::currentDateTime().toTime_t()
std::endl;
return app.exec();
}
diff --git a/ksmserver/screenlocker/ksldapp.cpp
b/ksmserver/screenlocker/ksldapp.cpp
index 7f2e671..6a6fad8 100644
--- a/ksmserver/screenlocker/ksldapp.cpp
+++ b/ksmserver/screenlocker/ksldapp.cpp
@@ -277,10 +277,13 @@ bool KSldApp::startLockProcess()
}
m_lockProcess-start();
// we wait one minute
-if (!m_lockProcess-waitForStarted()) {
+if (m_lockProcess-waitForStarted(6)) {
+m_lockProcess-waitForReadyRead(6);
+} else {
m_lockProcess-kill();
return false;
}
+
return true;
}
diff --git a/plasma/screensaver/shell/main.cpp
b/plasma/screensaver/shell/main.cpp
index be151aa..a70bcdf 100644
--- a/plasma/screensaver/shell/main.cpp
+++ b/plasma/screensaver/shell/main.cpp
@@ -21,6 +21,10 @@
#include KLocale
#include KIcon
+#include QDateTime
+
+#include iostream
+
#include config-workspace.h
#include plasmaapp.h
@@ -55,6 +59,9 @@ int main(int argc, char **argv)
KGlobal::locale()-insertCatalog(QLatin1String( libkworkspace ));
KGlobal::locale()-insertCatalog(QLatin1String( kscreenlocker_greet ));
app-disableSessionManagement(); // I assume we'd never want a screensaver
thing reppearing on login?
+// This allow ksmserver to know when the applicaion has actually finished
setting itself up.
+// Crucial for blocking until it is ready, ensuring locking happens before
sleep, e.g.
+std::cout Locked at QDateTime::currentDateTime().toTime_t()
std::endl;
int rc = app-exec();
delete app;
return rc;
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[kde-workspace] plasma/generic/applets/lock_logout: keep plugin name consistent between releases
Git commit dd23569be148fa900de5046871c703a2873b5eb8 by Aaron Seigo. Committed on 08/06/2012 at 22:19. Pushed by aseigo into branch 'master'. keep plugin name consistent between releases when rewriting plugins in QML they MUST keep the same plugin name or else people's configurations will become broken! CCMAIL:plasma-devel@kde.org BUG:301368 M +1-1plasma/generic/applets/lock_logout/metadata.desktop http://commits.kde.org/kde-workspace/dd23569be148fa900de5046871c703a2873b5eb8 diff --git a/plasma/generic/applets/lock_logout/metadata.desktop b/plasma/generic/applets/lock_logout/metadata.desktop index 341bfde..6cc4304 100644 --- a/plasma/generic/applets/lock_logout/metadata.desktop +++ b/plasma/generic/applets/lock_logout/metadata.desktop @@ -182,7 +182,7 @@ X-Plasma-DefaultSize=80,120 X-KDE-PluginInfo-Author=Viranch Mehta X-KDE-PluginInfo-Email=viranch.me...@gmail.com -X-KDE-PluginInfo-Name=org.kde.lockout +X-KDE-PluginInfo-Name=lockout X-KDE-PluginInfo-Version=1.0 X-KDE-PluginInfo-Website=http://plasma.kde.org/ X-KDE-PluginInfo-Category=System Information ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Rework KMix DBus API and add KMix plasma dataengine
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6587/#review9965
---
the DataEngine part looks good now.. nice work :)
i'll leave the review of the kmix parts to the kmix devs, though ...
- Aaron
On March 6, 2011, 7:24 p.m., Igor Poboiko wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6587/
---
(Updated March 6, 2011, 7:24 p.m.)
Review request for Plasma and Diego Casella.
Summary
---
This patch reworks KMix DBus API and adds a plasma dataengine+service as a
frontend to information provided by DBus.
New DBus structure is:
- /Mixers
used to get some global information, such as available mixers list and global
master mixer
- /Mixers/MIXER_ID
used to get information about mixer with id=MIXER_ID. It provides such
information as list of available controls, name of this mixer, id, etc
- /Mixers/MIXER_ID/CONTROL_ID
used to get and set information about control. Such information as volume
level, mute, name of control, etc.
It also adds a DBus signals which are emitted when new mixer/control appears,
or volume level changes.
It also splits all dbus-related code to separate class,
DBus{KMix,Mixer,Control}Wrapper.
The Plasma Dataengine:
By default, the only available source is KMix. It provides information
global information about KMix: is KMix running, and list of available mixers.
(its IDs)
Source for every mixer is called by it's ID (for example,
ALSA::HDA_Intel:1). This source provides such information about current
Mixer as: it's readable name, is it opened, its balance and list of available
controls. It also adds basic sources for every control, which provides only
information about its readable name
Sources for controls are called by 'MIXER_ID/CONTROL_ID' (for example,
ALSA::HDA_Intel:1/Master:0). If you request this source, it will provide
such information as its readable name, is it muted and its volume level
(which are updates automatically, using DBus signals).
There is a service available for controls sources. It provides such methods
as setVolume() and setMute().
It doesn't close bug 171287, but it becomes one step closer to its solving :)
And, I'm not very familiar with CMake, but it would be great idea to make
plasma part optional.
This addresses bug 171287.
https://bugs.kde.org/show_bug.cgi?id=171287
Diffs
-
/trunk/KDE/kdemultimedia/kmix/CMakeLists.txt 1223808
/trunk/KDE/kdemultimedia/kmix/apps/kmix.cpp 1223808
/trunk/KDE/kdemultimedia/kmix/core/mixdevice.h 1223808
/trunk/KDE/kdemultimedia/kmix/core/mixdevice.cpp 1223808
/trunk/KDE/kdemultimedia/kmix/core/mixer.h 1223808
/trunk/KDE/kdemultimedia/kmix/core/mixer.cpp 1223808
/trunk/KDE/kdemultimedia/kmix/dbus/dbuscontrolwrapper.h PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/dbuscontrolwrapper.cpp PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/dbusmixerwrapper.h PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/dbusmixerwrapper.cpp PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/dbusmixsetwrapper.h PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/dbusmixsetwrapper.cpp PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/org.kde.kmix.control.xml PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/org.kde.kmix.mixer.xml PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/dbus/org.kde.kmix.mixset.xml PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/CMakeLists.txt PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/CMakeLists.txt PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixer.operations PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.h PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerservice.h PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerservice.cpp PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/plasma/engine/plasma-engine-mixer.desktop
PRE-CREATION
/trunk/KDE/kdemultimedia/kmix/tests/CMakeLists.txt 1223808
Diff: http://svn.reviewboard.kde.org/r/6587/diff
Testing
---
KMix from KDE SC 4.6.0 compiles ok with this patch, and patch applies to
current trunk.
Tested on system with one card and with ALSA backend, so I don't know is
plasma dataengine works correctly with plugging/unplugging mixers (but it
should).
All DBus methods/properties works fine, signals are emitted, volume can be
set using DBus methods.
Plasma dataengine was tested using plasmaengineexplorer.
All works fine except the one thing. When I request an source for Mixer, it
also adds soucres for controls. And
Re: Review Request: Rework KMix DBus API and add KMix plasma dataengine
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6587/#review9951
---
When I request an source for Mixer, it also adds soucres for controls. And
then when I request source for already available Control, it doesn't react
anyhow. But when I set Update every % ms, and click Reqeust, it works fine.
this is because the data for the control (or at least, the readableName) is set
in updateMixerData. then, when it is later requested the DataEngine sees it
exists already and so does not call sourceRequestEvent. since there is no poll
(time) request, updateSourceEvent isn't called either. when there is a poll
time set, then updateSourceEvent is (eventually) called and the data is
updated. the fix is to not set any data on the control until such time as a
visualization requests it. you can set up the behind-the-scenes objects, but
leave the setData calls for the control out of updateMixerData.
there are some memory leaks that need closing as well.
i also recommend, for stylistic reasons, using natural language labels rather
than camelCaseAsIfItsAFunctionName labels. e.g.: Controls, Readable Name,
etc.
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11146
this is wrong. if the name is KMix then the source created _must_ be
KMix, but getKMixData creates/sets running and mixers
my suggestion: change this to if (name == mixers)
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11145
does this matter? if mixers is empty, shouldn't that be enough?
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11147
where is this deleted?
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11148
looks like a good candidate for a QHash rather than a QList
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11149
perhaps another opportunity for a hash.
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11151
another good candidate for a hash
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11150
where is this deleted?
/trunk/KDE/kdemultimedia/kmix/plasma/engine/mixerengine.cpp
http://svn.reviewboard.kde.org/r/6587/#comment11152
no data for the control should be set here. this belongs in
updateControlData
- Aaron
On March 5, 2011, 7:56 a.m., Igor Poboiko wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6587/
---
(Updated March 5, 2011, 7:56 a.m.)
Review request for Plasma and Diego Casella.
Summary
---
This patch reworks KMix DBus API and adds a plasma dataengine+service as a
frontend to information provided by DBus.
New DBus structure is:
- /Mixers
used to get some global information, such as available mixers list and global
master mixer
- /Mixers/MIXER_ID
used to get information about mixer with id=MIXER_ID. It provides such
information as list of available controls, name of this mixer, id, etc
- /Mixers/MIXER_ID/CONTROL_ID
used to get and set information about control. Such information as volume
level, mute, name of control, etc.
It also adds a DBus signals which are emitted when new mixer/control appears,
or volume level changes.
It also splits all dbus-related code to separate class,
DBus{KMix,Mixer,Control}Wrapper.
The Plasma Dataengine:
By default, the only available source is KMix. It provides information
global information about KMix: is KMix running, and list of available mixers.
(its IDs)
Source for every mixer is called by it's ID (for example,
ALSA::HDA_Intel:1). This source provides such information about current
Mixer as: it's readable name, is it opened, its balance and list of available
controls. It also adds basic sources for every control, which provides only
information about its readable name
Sources for controls are called by 'MIXER_ID/CONTROL_ID' (for example,
ALSA::HDA_Intel:1/Master:0). If you request this source, it will provide
such information as its readable name, is it muted and its volume level
(which are updates automatically, using DBus signals).
There is a service available for controls sources. It provides such methods
as setVolume() and setMute().
It doesn't close bug 171287, but it becomes one step closer to its solving :)
And, I'm not very familiar with CMake, but it would be great idea to make
plasma part optional.
Re: Review Request: ClockApplet : show seconds in the tooltip
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6336/#review9760 --- a good step in the right direction, but it's still needs work. looking at the approach taken, i'd probably do this: * rename ClockTimer to something clearer - ClockToolTip? * have ClockToolTip subclass both QObject and Plasma::ToolTipContent * move updateToolTip into ClockToolTip, renamed as dataUpdated(const QString source, const Plasma::DataEngine data) ;) * connect to the dataengine on creation of ClockToolTip * create a ClockToolTip object when toolTipAboutToShow is called * delete it in toolTipHidden * pass in the ClockApplet as the parent to ClockToolTip and then in ClockToolTip::dataUpdated, set the mainText and then call Plasma::ToolTipManager::self()-setContent(clockApplet, this) that means that ClockApplet::updateToolTip would no long be available to subclasses, but that's ok because they were only using it to update the tooltip when the time changed ... which ClockApplet would now be doing for them. /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp http://svn.reviewboard.kde.org/r/6336/#comment10905 this will interfere with any clock applet (which is all of them) which implements dataUpdated. moreover, calling updateTipContent without the data passed in, only to discard it seems wasteful. - Aaron On Jan. 29, 2011, 2:15 p.m., Iamluc wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6336/ --- (Updated Jan. 29, 2011, 2:15 p.m.) Review request for Plasma. Summary --- Hi, When using a clock applet, seconds are often not visibles. But sometimes you need them. This patch shows them in the tooltip of the applet. Analog-clock has been updated to refresh the tooltip every seconds. If this change is accepted, I could change digital-clock Thanks ! Diffs - /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.h 1217808 /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp 1217808 Diff: http://svn.reviewboard.kde.org/r/6336/diff Testing --- It Works on plasmoidviewer and with a real session Thanks, Iamluc ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Overrides Plasma::wallpaer method addUrls() to set Image
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6391/#review9725 --- Ship it! - Aaron On Jan. 25, 2011, 7:49 p.m., Sinny Kumari wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6391/ --- (Updated Jan. 25, 2011, 7:49 p.m.) Review request for Plasma. Summary --- Method addUrls(const KUrl::List urls) which is protected Q_SLOTS will override the method defined ( changes that have been made in http://svn.reviewboard.kde.org/r/6375/ ) in the base class Plasma::wallpaper to set Image. After reviewing it I will implement both changes in Frame Applet :) Diffs - trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.h 1216449 trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp 1216449 Diff: http://svn.reviewboard.kde.org/r/6391/diff Testing --- Tested in trunk and it works perfectly. Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Overrides Plasma::wallpaer method addUrls() to set Image
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6391/#review9669 --- trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp http://svn.reviewboard.kde.org/r/6391/#comment10777 if it is running as a slideshow, it should probably add all of the urls to the slideshow (not just the first one) and start showing the first of them. - Aaron On Jan. 21, 2011, 5:52 a.m., Sinny Kumari wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6391/ --- (Updated Jan. 21, 2011, 5:52 a.m.) Review request for Plasma. Summary --- Method addUrls(const KUrl::List urls) which is protected Q_SLOTS will override the method defined ( changes that have been made in http://svn.reviewboard.kde.org/r/6375/ ) in the base class Plasma::wallpaper to set Image. After reviewing it I will implement both changes in Frame Applet :) Diffs - trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.h 1215563 trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp 1215563 Diff: http://svn.reviewboard.kde.org/r/6391/diff Testing --- Tested in trunk and it works perfectly. Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make wallpaper capable of setting particular Image
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6375/ --- (Updated Jan. 20, 2011, 5:44 p.m.) Review request for Plasma. Summary --- According to suggestion given by Aaron (refer to mailing list thread http://mail.kde.org/pipermail/plasma-devel/2011-January/014610.html ), I have done changes in trunk/KDE/kdelibs/plasma/. I have added functions setUrls(const KUrl::List urls) as public function and addUrls(const KUrl::List urls) as protected Q_SLOTS in it so that setUrls() can be invoked from frame applet. There are more patches to come, after this one gets approved. Diffs - trunk/KDE/kdelibs/plasma/wallpaper.h 1215563 trunk/KDE/kdelibs/plasma/wallpaper.cpp 1215563 Diff: http://svn.reviewboard.kde.org/r/6375/diff Testing --- Tested in trunk and everything is working fine. Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make wallpaper capable of setting particular Image
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6375/#review9658 --- Ship it! just needs apidox. trunk/KDE/kdelibs/plasma/wallpaper.h http://svn.reviewboard.kde.org/r/6375/#comment10728 will need API documentation before committing. trunk/KDE/kdelibs/plasma/wallpaper.h http://svn.reviewboard.kde.org/r/6375/#comment10729 needs apidox; should include a bit about how in libplasma2/KDE5 this will be merged with setUrls and be made a virtual method. will need an @since 4.7 in there too :) - Aaron On Jan. 20, 2011, 5:44 p.m., Sinny Kumari wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6375/ --- (Updated Jan. 20, 2011, 5:44 p.m.) Review request for Plasma. Summary --- According to suggestion given by Aaron (refer to mailing list thread http://mail.kde.org/pipermail/plasma-devel/2011-January/014610.html ), I have done changes in trunk/KDE/kdelibs/plasma/. I have added functions setUrls(const KUrl::List urls) as public function and addUrls(const KUrl::List urls) as protected Q_SLOTS in it so that setUrls() can be invoked from frame applet. There are more patches to come, after this one gets approved. Diffs - trunk/KDE/kdelibs/plasma/wallpaper.h 1215563 trunk/KDE/kdelibs/plasma/wallpaper.cpp 1215563 Diff: http://svn.reviewboard.kde.org/r/6375/diff Testing --- Tested in trunk and everything is working fine. Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: ClockApplet : show seconds in the tooltip
On Jan. 15, 2011, 8:08 p.m., Aaron Seigo wrote: i agree with Marco, that this should end up in the ClockApplet class shared by all clocks. the way this probably will need to be done is to have a QObject with a dataUpdated method used for this purpose in ClockApplet that is connected/disconnected to the time engine using the current timezone on toolTipAboutToShow/Hide. then it will work with all clocks. Marco Martin wrote: yeah, i like the idea. this maybe suggests it's also time to make DataEngineConsumer public API in libplasma. make DataEngineConsumer public API in libplasma yes, it could be. taking it one step further: perhaps in libplasma2 DataEngineManager goes private (further protecting the usage of DataEngines; we've seen some bugs in C++ in the past due to using DataEngineManager directly and incorrectly) and DataEngineConsumer becomes a public interface instead. that said, in this case, ClockApplet can simply pass the return from dataEngine(time) into the QObject. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6336/#review9635 --- On Jan. 15, 2011, 3:17 p.m., Iamluc wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6336/ --- (Updated Jan. 15, 2011, 3:17 p.m.) Review request for Plasma. Summary --- Hi, When using a clock applet, seconds are often not visibles. But sometimes you need them. This patch shows them in the tooltip of the applet. Analog-clock has been updated to refresh the tooltip every seconds. If this change is accepted, I could change digital-clock Thanks ! Diffs - /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp 1214556 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.h 1214556 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.cpp 1214556 Diff: http://svn.reviewboard.kde.org/r/6336/diff Testing --- It Works on plasmoidviewer and with a real session Thanks, Iamluc ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Mime-type in NepomukSearchRunner
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6338/#review9639 --- some code style issues, but otherwise looks good. /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/nepomuksearchrunner.cpp http://svn.reviewboard.kde.org/r/6338/#comment10664 these should go at the top of the file with the other includes. preferably using the CamelCase style, e.g. #include KMimeType /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/nepomuksearchrunner.cpp http://svn.reviewboard.kde.org/r/6338/#comment10665 no spaces before/after ()s /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/nepomuksearchrunner.cpp http://svn.reviewboard.kde.org/r/6338/#comment10666 whitespace /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/queryclientwrapper.cpp http://svn.reviewboard.kde.org/r/6338/#comment10667 whitespace after (s and before )s should be removed - Aaron On Jan. 16, 2011, 9:47 a.m., Amir Pakdel wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6338/ --- (Updated Jan. 16, 2011, 9:47 a.m.) Review request for Plasma, Sebastian Trueg and Vishesh Handa. Summary --- I have changed NepomukSearchRunner a little bit, so I could search for contents of notes (added by BasketNotepad) using KRunner. It uses NIE:mimeType of the resource to set the type and iconName of the search result. If the resourse does not have NIE:mimeType property, KMimeType::findByUrl(url) is used instead (in order to find the mime-type associated with the url of the resource). When running/opening the search result, preferredService of the mime-type is used in invokation of KRun::run; therefore, the mime-type of the resource (or its url) indicates the service that would be used to run the resource. Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/nepomuksearchrunner.cpp 1193800 /trunk/KDE/kdebase/workspace/plasma/generic/runners/nepomuksearch/queryclientwrapper.cpp 1193800 Diff: http://svn.reviewboard.kde.org/r/6338/diff Testing --- I have searched for contents of notes that were added by BasketNotepad (using KRunner) and opened them in BasketNotepads. Thanks, Amir ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Tasks applet: Make order independent of whether the row count is forced.
On Jan. 10, 2011, 10 a.m., Kåre Särs wrote: There is a problem with the new behavior for forced row count. Previously the older windows/buttons stayed at approximately the same place when new windows where opened/closed, but now more than half of the windows jump back and forth whenever a window is opened or closed. I made a longer explanation of my problem with the new behavior in the bug report that this patch tries to solve. Should I maybe re-open the report? Regards, Kåre each way has its strengths and its challenges. adding an option to swap between the two ways is not a good solution in this case, so we need to pick one. i'm also uninterested in having it flip-flop back and forth in implementation. the way it is currently in 4.6 seems to cover a number of user needs very well. can't please everyone, as they say. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/#review9601 --- On Nov. 6, 2010, 8:58 p.m., Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/ --- (Updated Nov. 6, 2010, 8:58 p.m.) Review request for Plasma. Summary --- This should fix https://bugs.kde.org/show_bug.cgi?id=215231, but frankly I don't understand why it was done this way in the first place... This addresses bug 215231. https://bugs.kde.org/show_bug.cgi?id=215231 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp 1190987 Diff: http://svn.reviewboard.kde.org/r/5776/diff Testing --- Screenshots --- 3 forced rows, 1-5 windows, trunk vs. patched http://svn.reviewboard.kde.org/r/5776/s/550/ Thanks, Ingomar ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Obey locale KCM settings for data format in the Digital Clock Plasma applet
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6272/#review9522
---
a few issues, mostly simple things about whitespacing, but one significant
issue with the launching of the kcm. once that is fixed up, please commit :)
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10521
whitespace, should be: if (cg.hasKey(showYear)) {
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10523
whitspace; also {} are missing (we use them around even single-line if
statements)
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10524
probably unecessary; doesn't do any harm to have it sitting there, so no
reason to spend cycles on it or leave code related to it there for us to figure
out later if we should keep it :)
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10526
whitespace: no spaces after '(' or before ')'
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10525
this warning already occurs in the kcm in KCMLocale::save(), though it is
currently only triggered on language changes. should probably be also cover
dates.
this warning isn't needed here, however, but in the kcm.
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10527
} else if (m_dateStyle == 2) {
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6272/#comment10528
exec() is blocking, which means it will hang the rest of plasma-desktop, so
that can't happen.
instead, try this:
KService::List offers = KServiceTypeTrader::self()-query(KCModule,
Library == 'kcm_locale');
if (!offers.isEmpty()) {
KService::Ptr service = offers.first();
KRun::run(*service, KUrl::List(), 0);
}
- Aaron
On 2011-01-04 19:47:45, Teo Mrnjavac wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6272/
---
(Updated 2011-01-04 19:47:45)
Review request for Plasma.
Summary
---
Replaces the hardcoded date formats in the digital clock applet config dialog
with the following choices:
* No date
* Compact date
* Short date (obeys the KCM)
* Long date (obeys the KCM)
* ISO date
Also adds a button to launch the country/region and language settings KCM.
This addresses bugs 195837 and 247277.
https://bugs.kde.org/show_bug.cgi?id=195837
https://bugs.kde.org/show_bug.cgi?id=247277
Diffs
-
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/CMakeLists.txt
1211741
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.h
1211741
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp
1211741
/trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clockConfig.ui
1211741
Diff: http://svn.reviewboard.kde.org/r/6272/diff
Testing
---
During and after coding.
Thanks,
Teo
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Lockout widget: Explain to the user that he must select a button to show
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/#review9400 --- Ship it! - Aaron On 2010-12-05 21:41:56, Nicolas Lécureuil wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/ --- (Updated 2010-12-05 21:41:56) Review request for Plasma. Summary --- When the user ask to show no buttons in the lockout widget, nothing tell the user that this is not possible, and when he saves the configuration nothing changed in the widget. With this patch, if the user unselect all the checkboxes, he is warned. This addresses bug 256879. https://bugs.kde.org/show_bug.cgi?id=256879 Diffs - trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.h 1203948 trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp 1203948 Diff: http://svn.reviewboard.kde.org/r/6041/diff Testing --- Thanks, Nicolas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fix crash in plasmapkg
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6184/#review9371
---
Ship it!
- Aaron
On 2010-12-22 13:35:33, Nicolas Lécureuil wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6184/
---
(Updated 2010-12-22 13:35:33)
Review request for Plasma.
Summary
---
when using plasmapkg -t theme -u
/home/neoclust/.kde4/share/apps/desktoptheme/G-Remix/metadata.desktop (
which is a wrong use of the tool i agree ) plasmapkg crash.
this is because archive is set to 0 and not checked before if
(archive-open(QIODevice::ReadOnly)) {
this patch fixes this crash
Diffs
-
trunk/KDE/kdelibs/plasma/packagestructure.cpp 1208544
Diff: http://svn.reviewboard.kde.org/r/6184/diff
Testing
---
plasmapkg -t theme -u ${the file you want}
Thanks,
Nicolas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: plasma (PackageStructurePrivate::createPackageMetadata) : make a kWarning be usefull again
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6185/#review9372 --- Ship it! good catches, thanks :) - Aaron On 2010-12-22 14:08:04, Nicolas Lécureuil wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6185/ --- (Updated 2010-12-22 14:08:04) Review request for Plasma. Summary --- in PackageStructurePrivate::createPackageMetadata there is a kWarning which is useless because the stream to show is cleared just before the kWarning. This small patch fixes this issue. Diffs - trunk/KDE/kdelibs/plasma/packagestructure.cpp 1208544 Diff: http://svn.reviewboard.kde.org/r/6185/diff Testing --- Thanks, Nicolas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: The clock applet chiming per hour, half-hour and quarter hour
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6108/ --- (Updated 2010-12-13 19:17:35.920939) Review request for Plasma and Anne-Marie Mahfouf. Summary --- Hello Everybody, i have implemented the chiming of the analog clock every hour.though i have hard coded it and it would only chime every hour. and not for 45 mins. Presently I am working on the development of a ui which would allow the user to set the clock to chime according to the choice of the user. thanks, sunny_slls This addresses bug 232004. https://bugs.kde.org/show_bug.cgi?id=232004 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/CMakeLists.txt 1203585 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.h 1203585 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.cpp 1203585 Diff: http://svn.reviewboard.kde.org/r/6108/diff Testing --- Thanks, Sunny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Use correct pixmap cache file for theme
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6100/#review9239 --- Ship it! ah, that is sooo much clearer with the enumeration. thanks :) - Aaron On 2010-12-13 19:55:48, Manuel Mommertz wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6100/ --- (Updated 2010-12-13 19:55:48) Review request for Plasma. Summary --- Currently when switching themes, the ondisk cache for pixmaps don't switch to the correct file. This leads to not updated SVGs on theme change. (For some reason folderview doesn't update. But currently no graphic updates.) Diffs - /trunk/KDE/kdelibs/plasma/theme.cpp 1206183 Diff: http://svn.reviewboard.kde.org/r/6100/diff Testing --- Switching themes, system colors and compositing. Thanks, Manuel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Use correct pixmap cache file for theme
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6100/#review9220
---
i agree with the analysis of the problem: the pixmapCache object is not always
reset in ThemePrivate::setThemeName. there some issues with the implementation,
however, that need to be addressed before it can be committed.
/trunk/KDE/kdelibs/plasma/theme.cpp
http://svn.reviewboard.kde.org/r/6100/#comment10089
whitespace, should be: if (themeName
/trunk/KDE/kdelibs/plasma/theme.cpp
http://svn.reviewboard.kde.org/r/6100/#comment10090
whitespace: if (info
/trunk/KDE/kdelibs/plasma/theme.cpp
http://svn.reviewboard.kde.org/r/6100/#comment10092
this method's signature is leading to hard to read code and is going to be
error prone. if we indeed move to a two-boolean method here, i'd prefer to see
an enumeration used as flags in ThemePrivate. e.g.
enum { NoCache = 0, PixmapCache = 1, SvgElementsCache = 2 } CacheType;
Q_DECLARE_FLAGS(CacheTypes, CacheType)
void ThemePrivate::discardCache(CacheTyeps caches)
and then calls would become sth like:
discardCache(PixmapCache | SvgElementsCache)
this would reverse the meaning the the current booleans, but then the code
becomes far easier to read as a result.
/trunk/KDE/kdelibs/plasma/theme.cpp
http://svn.reviewboard.kde.org/r/6100/#comment10087
it is unintuitive that a boolean called keepPixmapCache results in the
pixmap cache being deleted.
a comment should be included here noting that deleting the pixmapCache
object does not remove the on-disk backing.
/trunk/KDE/kdelibs/plasma/theme.cpp
http://svn.reviewboard.kde.org/r/6100/#comment10088
this may as well just be an else if (pixmapCache)
- Aaron
On 2010-12-12 16:31:51, Manuel Mommertz wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6100/
---
(Updated 2010-12-12 16:31:51)
Review request for Plasma.
Summary
---
Currently when switching themes, the ondisk cache for pixmaps don't switch to
the correct file. This leads to not updated SVGs on theme change.
(For some reason folderview doesn't update. But currently no graphic updates.)
Diffs
-
/trunk/KDE/kdelibs/plasma/theme.cpp 1205716
Diff: http://svn.reviewboard.kde.org/r/6100/diff
Testing
---
Switching themes, system colors and compositing.
Thanks,
Manuel
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: The clock applet chiming per hour, half-hour and quarter hour
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6108/ --- (Updated 2010-12-12 23:01:41.350300) Review request for Plasma and Anne-Marie Mahfouf. Summary --- Hello Everybody, i have implemented the chiming of the analog clock every hour.though i have hard coded it and it would only chime every hour. and not for 45 mins. Presently I am working on the development of a ui which would allow the user to set the clock to chime according to the choice of the user. thanks, sunny_slls This addresses bug https://bugs.kde.org/show_bug.cgi?id=232004. https://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=232004 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/CMakeLists.txt 1203585 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.h 1203585 /trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.cpp 1203585 Diff: http://svn.reviewboard.kde.org/r/6108/diff Testing --- Thanks, Sunny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: The clock applet chiming per hour, half-hour and quarter hour
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6108/#review9223
---
as others noted, the debug should be removed for the final patch; but we're not
quite there yet :)
this code really belongs, as John Layt noted, in libplasmaclock. in
kdebase/workspace/libs/plasmaclock/clockapplet.cpp there is a updateClockApplet
method that now calls speakTime. this feature should be handled similarly,
perhaps with a chimeTime() method.
/trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/CMakeLists.txt
http://svn.reviewboard.kde.org/r/6108/#comment10094
shouldn't be in the final patch :)
/trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.cpp
http://svn.reviewboard.kde.org/r/6108/#comment10095
whitespace: } else {
- Aaron
On 2010-12-12 23:01:41, Sunny Sharma wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6108/
---
(Updated 2010-12-12 23:01:41)
Review request for Plasma and Anne-Marie Mahfouf.
Summary
---
Hello Everybody,
i have implemented the chiming of the analog clock every hour.though i have
hard coded it and it would only chime every hour. and not for 45 mins.
Presently I am working on the development of a ui which would allow the user
to set the clock to chime according to the choice of the user.
thanks,
sunny_slls
This addresses bug https://bugs.kde.org/show_bug.cgi?id=232004.
https://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=232004
Diffs
-
/trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/CMakeLists.txt
1203585
/trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.h
1203585
/trunk/KDE/kdebase/workspace/plasma/generic/applets/analog-clock/clock.cpp
1203585
Diff: http://svn.reviewboard.kde.org/r/6108/diff
Testing
---
Thanks,
Sunny
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Contact runner works correctly in more cases
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6089/#review9203 --- Ship it! - Aaron On 2010-12-11 12:54:08, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6089/ --- (Updated 2010-12-11 12:54:08) Review request for Plasma, Aaron Seigo and Teemu Rytilahti. Summary --- Works around BUG:249157 by using the form NAME EMAIL instead of NAMEEMAIL. The first version is perfectly valid, the second that does not always work [1] might also be valid. [1] Fails e.g. for NAME=a ü-a or ü-a CCBUG:249157 Diffs - /trunk/KDE/kdeplasma-addons/runners/contacts/contactsrunner.cpp 1205477 Diff: http://svn.reviewboard.kde.org/r/6089/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Separate caching for unthemed SVGs requesting colorization
On 2010-12-05 14:50:33, Manuel Mommertz wrote: It is now even worse then before. Unthemed graphics still use the colors from plasma theme but don't get informed if the theme changes. this means, the colors that are used when first rendering the graphics stay even if I switch the theme. The plasma elements itself don't get rerendered on theme change. Only graphics that have to be rerendered for another reason (resizing a widget, ...) get the svg's from the new activated theme. Marco Martin wrote: well, they shouldn't be informed on theme changes in this case... Manuel Mommertz wrote: right, but on colorscheme changes. with doesn't happen. Unthemed graphics still use the colors from plasma theme but don't get informed if the theme changes. unthemed graphics never use colors from the plasma theme. they either use no colors, or they use the system colors. in which case are unthemed graphics using the color scheme from the plasma theme? the colors that are used when first rendering the graphics stay even if I switch the theme. of course. the theme is not relevant. you get to pick between themed and unthemed. The plasma elements itself don't get rerendered on theme change. which plasma elements? Only graphics that have to be rerendered for another reason (resizing a widget, ...) get the svg's from the new activated theme. are you saying that NO svg's (themed or unthemed) are being updated? because that works here. right, but on colorscheme changes. with doesn't happen. do you mean the KDE color scheme change? e.g. when you change the colorscheme in the Colors control panel? i'm quite confused now by what you mean. can you give me a clear, simple use case with detailed steps of reproduction so that i may identify and reproduce the problem and subsequently fix it? thanks. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6008/#review9136 --- On 2010-11-28 23:52:16, Aaron Seigo wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6008/ --- (Updated 2010-11-28 23:52:16) Review request for Plasma. Summary --- see: http://www.mail-archive.com/plasma-devel@kde.org/msg13494.html this introduces a Theme object used by Svg to cache the rendering of svg's asking to be colorized, but which are not part of the theme. this requires an independent cache and set of color files, therefore a second Theme object. SvgPrivate::actualTheme() is thus overridden by SvgPrivate::cacheAndColorsTheme() whenever caching or colors are used. there are two remaining defects of varying severity, though neither is very high imho: a) if the system color palette changes when the application is not running (or an svg using such a theme is not running) the cache will not be dropped. this is an existing issue, however, not something introduce by this patchset b) the new system color theme is shared for performance reasons, but once created it is never deleted. it ought to be reference counted so it can be cleaned up after in use, but this is currently not done. several other fixes crept into this patchset (sorry :/) including: * not dropping the cache just because we change themes; this made sense when just plasma-desktop used this or when the apps using Plasma::Theme all used the same theme. this is no longer the case, really, and having applications changing themes randomly kicking the cache out from underneath other apps that may still be using it is undesirable. this does mean that cache files will accumulate. not sure that's a big issue as they don't tend to be large and are in an area marked as cache (so ripe for clean-without-care) * consolidate the signal/slot connections in SvgPrivate::checkColorHints, and now the check is consistent on all paths (previously, it wasn't!) * missing (and possible cause of cache key ambiguity) separator in CACHE_ID_WITH_SIZE * cleanups such as getting rid of superfluous use of Plasma:: namespacing in code that is now in libplasma, such as the stylesheeting. (it was from a plasmoid sebas wrote originally, iirc, explaining the historical reason for the explicit namespacing) in any case, consider this a draft at this stage. i'm interested in geting feedback, improvements and testing. Diffs - /trunk/KDE/kdelibs/plasma/private/svg_p.h 1199366 /trunk/KDE/kdelibs/plasma/svg.cpp 1201684 /trunk/KDE/kdelibs/plasma/theme.cpp 1201685 Diff: http://svn.reviewboard.kde.org/r/6008/diff Testing --- just running normal plasma-desktop and what i use. needs testing with svg's that will actually rely
Re: Review Request: Separate caching for unthemed SVGs requesting colorization
On 2010-12-05 14:50:33, Manuel Mommertz wrote: It is now even worse then before. Unthemed graphics still use the colors from plasma theme but don't get informed if the theme changes. this means, the colors that are used when first rendering the graphics stay even if I switch the theme. The plasma elements itself don't get rerendered on theme change. Only graphics that have to be rerendered for another reason (resizing a widget, ...) get the svg's from the new activated theme. Marco Martin wrote: well, they shouldn't be informed on theme changes in this case... Manuel Mommertz wrote: right, but on colorscheme changes. with doesn't happen. Aaron Seigo wrote: Unthemed graphics still use the colors from plasma theme but don't get informed if the theme changes. unthemed graphics never use colors from the plasma theme. they either use no colors, or they use the system colors. in which case are unthemed graphics using the color scheme from the plasma theme? the colors that are used when first rendering the graphics stay even if I switch the theme. of course. the theme is not relevant. you get to pick between themed and unthemed. The plasma elements itself don't get rerendered on theme change. which plasma elements? Only graphics that have to be rerendered for another reason (resizing a widget, ...) get the svg's from the new activated theme. are you saying that NO svg's (themed or unthemed) are being updated? because that works here. right, but on colorscheme changes. with doesn't happen. do you mean the KDE color scheme change? e.g. when you change the colorscheme in the Colors control panel? i'm quite confused now by what you mean. can you give me a clear, simple use case with detailed steps of reproduction so that i may identify and reproduce the problem and subsequently fix it? thanks. Manuel Mommertz wrote: http://img502.imageshack.us/img502/2729/plasmadesktopxk7517.png Steps I did before taking this screenshot: Logged in while Oxygen was the selected theme. Switched theme to Air. Noticed: After switching all graphics stay unchanged. The folderview changed after some 10-20 seconds. The handle (seen next to folderview) changed to air after I unhovered folderview and hovered it again. Panel stays unchanged. The Entry in the taskbar gets the hover-graphic from air while falling back to oxygen when mouse leaves the entry. Windeco still uses oxywin colors. What I did to ensure this is not some strange local thing: Logged out Removed cache files from themes (oxygen and air, each two files. No other files were there from plasma) Removed kde-install-path Reinstalled Logged in again Nothing changed What I am going to do now: rebuild kdelibs and kdebase and test again none of this had anything to do with these changes, however. these were all little bugs that had crept into the codebase around repainting when the theme changes. i've fixed the ones i've seen so far already. but again, nothing to do with these changes. they would have been broken before this commit, too. what is broken (so i can reproduce, test and fix) with unthemed svgs? - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6008/#review9136 --- On 2010-11-28 23:52:16, Aaron Seigo wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6008/ --- (Updated 2010-11-28 23:52:16) Review request for Plasma. Summary --- see: http://www.mail-archive.com/plasma-devel@kde.org/msg13494.html this introduces a Theme object used by Svg to cache the rendering of svg's asking to be colorized, but which are not part of the theme. this requires an independent cache and set of color files, therefore a second Theme object. SvgPrivate::actualTheme() is thus overridden by SvgPrivate::cacheAndColorsTheme() whenever caching or colors are used. there are two remaining defects of varying severity, though neither is very high imho: a) if the system color palette changes when the application is not running (or an svg using such a theme is not running) the cache will not be dropped. this is an existing issue, however, not something introduce by this patchset b) the new system color theme is shared for performance reasons, but once created it is never deleted. it ought to be reference counted so it can be cleaned up after in use, but this is currently not done. several other fixes crept into this patchset
Re: Review Request: Allows umlauts and other non-ASCII symbols in the locations runner for URLs + work again with mailto: etc.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6042/#review9127 --- Ship it! /trunk/KDE/kdebase/workspace/plasma/generic/runners/locations/locationrunner.cpp http://svn.reviewboard.kde.org/r/6042/#comment9954 const int - Aaron On 2010-12-04 14:24:09, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6042/ --- (Updated 2010-12-04 14:24:09) Review request for Plasma and Aaron Seigo. Summary --- Allows a wider range of urls without a protocol to be accepted, like those containing umlauts etc., queries, or IPV4-Adresses. If no protocol has be defined, then ports aren't supported. BUG:227380 Correctly handle protocols that are not a NetorkLocation or http, ftp. BUG:228017 This addresses bugs 227380 and 228017. https://bugs.kde.org/show_bug.cgi?id=227380 https://bugs.kde.org/show_bug.cgi?id=228017 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/runners/locations/locationrunner.cpp 1203489 Diff: http://svn.reviewboard.kde.org/r/6042/diff Testing --- More Urls: Tried with different urls, including the one of the bug report www.hörstreich.de, google.at, 74.125.39.99/search?hl=enbiw=1280bih=891q=kdeaq=faqi=g10aql=oq=gs_rfai= etc. Problem: There is one problem though that is unrelated to this patch and did _not_ exist in KDE 4.5.4 If I enter google.at it will display the result that I can go to http://google.at, yet if I add :\d e.g. google.at:8 the result won't be updated as nothing matches, yet the old result still stays. In KDE 4.5.4 the old result would also disappear after a short time. Correctly handle Protocls: Tried mailto:ch...@example.com, mailto:info...@example.com?subject=current-issue and ~/kde. Problem: Again this problem existed before this patch. In BUG:167862 there is mentioned, that simply entering a folder name in your $HOME-dir displays the folder, so instead of ~/kde also kde should work. Though this is not the case. Instead when the Nepomuk Desktop Search Runner is activated just inputing kde works, unless it is excluded from indexing. Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Supports bookmarks that have no protocol set.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6045/#review9128 --- Ship it! /trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp http://svn.reviewboard.kde.org/r/6045/#comment9955 const int /trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp http://svn.reviewboard.kde.org/r/6045/#comment9956 const int - Aaron On 2010-12-04 15:56:49, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6045/ --- (Updated 2010-12-04 15:56:49) Review request for Plasma and Aaron Seigo. Summary --- Supports bookmarks that have no protocol set. BUG:228454 This addresses bug 228454. https://bugs.kde.org/show_bug.cgi?id=228454 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1203528 Diff: http://svn.reviewboard.kde.org/r/6045/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Lockout widget: Explain to the user that he must select a button to show
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/#review9129 --- i think it does need a string to explain, because unless one understands _why_ it is doing such a thing it could appear pretty magical. imho this is something that can wait for 4.7 because, while odd and perhaps even annoying, it doesn't result in any data loss. a simple Select one or more items from the list below string would probably help a lot. my other concern with putting this in without a string is that, with the bug fixed, we will forget to go in and add the string when we're open for string freeze. all imho. - Aaron On 2010-12-04 15:00:02, Nicolas Lécureuil wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/ --- (Updated 2010-12-04 15:00:02) Review request for Plasma. Summary --- When the user ask to show no buttons in the lockout widget, nothing tell the user that this is not possible, and when he saves the configuration nothing changed in the widget. With this patch, if the user unselect all the checkboxes, he is warned. This addresses bug 256879. https://bugs.kde.org/show_bug.cgi?id=256879 Diffs - trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.h 1202613 trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp 1202613 Diff: http://svn.reviewboard.kde.org/r/6041/diff Testing --- Thanks, Nicolas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Lockout widget: Explain to the user that he must select a button to show
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/#review9121 --- unfortunately, a modal dialog like KMessageBox can't be used since it will block the rest of the UI. instead, it would probably be better if there was a text in the dialog itself (so the user knows ahead of time what they must do) and simply prevent the last item from becoming unchecked (so if the # of selected buttons == 1, don't let it become unselected). note that we are also in string freeze, so this probably needs to wait for 4.7. trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp http://svn.reviewboard.kde.org/r/6041/#comment9951 method start with lower case letters; probably just buttonsChanged() in this case trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp http://svn.reviewboard.kde.org/r/6041/#comment9950 no spaces within ()s. trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp http://svn.reviewboard.kde.org/r/6041/#comment9949 this will block the UI, which can't happen in plasma-desktop. - Aaron On 2010-12-04 01:58:14, Nicolas Lécureuil wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6041/ --- (Updated 2010-12-04 01:58:14) Review request for Plasma. Summary --- When the user ask to show no buttons in the lockout widget, nothing tell the user that this is not possible, and when he saves the configuration nothing changed in the widget. With this patch, if the user unselect all the checkboxes, he is warned. This addresses bug 256879. https://bugs.kde.org/show_bug.cgi?id=256879 Diffs - trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.h 1202613 trunk/KDE/kdebase/workspace/plasma/generic/applets/lock_logout/lockout.cpp 1202613 Diff: http://svn.reviewboard.kde.org/r/6041/diff Testing --- Thanks, Nicolas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: BookmarksRunner uses better way to find the default browser
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6032/#review9099 --- Ship it! the mechanism for storing these preferences is so amazingly ugly :/ i've wanted default-apps-stored-in-mimetype-db for going on 6 years now, and this is a great example of why. *sigh* anyways looks ok ... /trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp http://svn.reviewboard.kde.org/r/6032/#comment9894 if (service) is clearer? - Aaron On 2010-12-02 19:20:32, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6032/ --- (Updated 2010-12-02 19:20:32) Review request for Plasma and Aaron Seigo. Summary --- If no specific browser has been set as default, uses the application that is set to open html-files. Ideally there would be a method in kdelibs to retrieve the default browser though I suppose that is a different task than this bug. This addresses bug 224406. https://bugs.kde.org/show_bug.cgi?id=224406 Diffs - /trunk/KDE/kdebase/apps/dolphin/src/tests/dolphinviewtest_allviewmodes.cpp 1202673 /trunk/KDE/kdebase/apps/dolphin/src/tests/testbase.cpp 1202673 /trunk/KDE/kdebase/runtime/nepomuk/services/backupsync/gui/nepomukbackup.desktop 1202673 /trunk/KDE/kdebase/runtime/plasma/containments/newspaper/appletoverlay.cpp 1202673 /trunk/KDE/kdebase/runtime/plasma/containments/newspaper/appletsview.cpp 1202673 /trunk/KDE/kdebase/runtime/plasma/declarativeimports/core/theme.cpp 1202673 /trunk/KDE/kdebase/runtime/plasma/declarativeimports/core/theme_p.h 1202673 /trunk/KDE/kdebase/workspace/plasma/desktop/shell/activitymanager/filterbar.cpp 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/containmentactions/switchactivity/switch.cpp 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/containmentactions/switchwindow/switch.cpp 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/dataengines/activities/activities.operations 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/dataengines/activities/activityengine.cpp 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/dataengines/activities/activityjob.cpp 1202673 /trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1202673 Diff: http://svn.reviewboard.kde.org/r/6032/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Asynchronously show the KSNI context menu to avoid crashes
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6020/#review9081 --- Ship it! make sense :) - Aaron On 2010-12-01 17:06:19, Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6020/ --- (Updated 2010-12-01 17:06:19) Review request for Plasma and Marco Martin. Summary --- In some situations (KPackageKit KSNI comes to mind), the KSNI is deleted when the user right clicks it. While the behavior of that application is questionable, it should not crash the workspace. The change here ensures that the menu is showing asynchronously. It prevents a random crash which I uncovered after fixing the original issue of bug #241562 (see http://reviewboard.kde.org/r/6022/ ) This addresses bug 241562. https://bugs.kde.org/show_bug.cgi?id=241562 Diffs - trunk/KDE/kdebase/workspace/plasma/generic/applets/systemtray/protocols/dbussystemtray/dbussystemtraywidget.cpp 1201811 Diff: http://svn.reviewboard.kde.org/r/6020/diff Testing --- Right-clicking KPackageKit KSNI would cause a crash one time out of three before this patch. It does not crash anymore now. Thanks, Aurélien ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Notes widget: Working bold/italic/underline/strike out even when the note text is empty
On 2010-11-30 01:04:03, Davide Bettio wrote: Should we apply this patch while waiting someone who fix this bug in Qt? sure (if only because the bug has been closed as out of scope), but annotate the code with a comment explaining the situation and a link to the bug report on the qt bug tracker. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6005/#review9050 --- On 2010-11-28 20:23:45, Davide Bettio wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6005/ --- (Updated 2010-11-28 20:23:45) Review request for Plasma and Stephen Kelly. Summary --- Text format of the note can't be changed while the note is empty, so I add a space and I change the text format. This work around is rather stupid but I don't have any nicer idea. Anyway this might be a bug in KRichTextEdit so I add to this review request also steveire. Diffs - trunk/KDE/kdeplasma-addons/applets/notes/notes.h 1201054 trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp 1201054 Diff: http://svn.reviewboard.kde.org/r/6005/diff Testing --- Thanks, Davide ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fixes QuickSand scrolling animation
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5988/#review9015 --- Ship it! looks like it also cleans up some dubious memory management with the time line creation. - Aaron On 2010-11-27 21:04:37, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5988/ --- (Updated 2010-11-27 21:04:37) Review request for Plasma. Summary --- If an animation is already running finish it before starting a new one. BUG:209485 Diffs - /trunk/KDE/kdebase/workspace/krunner/interfaces/quicksand/qs_matchview.h 1201458 /trunk/KDE/kdebase/workspace/krunner/interfaces/quicksand/qs_matchview.cpp 1201458 Diff: http://svn.reviewboard.kde.org/r/5988/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: The KSystemActivityDialog uses MB as default unit.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6007/ --- (Updated 2010-11-28 23:31:11.663968) Review request for Plasma, Aaron Seigo and John Tapsell. Summary --- The KSystemActivityDialog uses MB as default unit. BUG:222022 This addresses bug 222022. https://bugs.kde.org/show_bug.cgi?id=222022 Diffs - /trunk/KDE/kdebase/workspace/krunner/ksystemactivitydialog.cpp 1201754 Diff: http://svn.reviewboard.kde.org/r/6007/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: The KSystemActivityDialog uses MB as default unit.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6007/#review9030 --- i think the real problem is in KSysGuardProcessList::loadSettings(const KConfigGroup cg) where it uses hard coded defaults such as this line: setUnits((ProcessModel::Units) cg.readEntry(units, (int)ProcessModel::UnitsKB)); it should be possible to do: m_processList.setUnits(ProcessModel::UnitsMB); m_processList.loadSettings(config); and have the current settings used as the defaults. the hardcoded defaults can be set up in the ctor of KSysGuardProcessList. so the setUnits line above would become: setUnits((ProcessModel::Units) cg.readEntry(units, (int)d-m_model.units())); thoughts? - Aaron On 2010-11-28 23:31:11, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6007/ --- (Updated 2010-11-28 23:31:11) Review request for Plasma, Aaron Seigo and John Tapsell. Summary --- The KSystemActivityDialog uses MB as default unit. BUG:222022 This addresses bug 222022. https://bugs.kde.org/show_bug.cgi?id=222022 Diffs - /trunk/KDE/kdebase/workspace/krunner/ksystemactivitydialog.cpp 1201754 Diff: http://svn.reviewboard.kde.org/r/6007/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Separate caching for unthemed SVGs requesting colorization
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6008/ --- Review request for Plasma. Summary --- see: http://www.mail-archive.com/plasma-devel@kde.org/msg13494.html this introduces a Theme object used by Svg to cache the rendering of svg's asking to be colorized, but which are not part of the theme. this requires an independent cache and set of color files, therefore a second Theme object. SvgPrivate::actualTheme() is thus overridden by SvgPrivate::cacheAndColorsTheme() whenever caching or colors are used. there are two remaining defects of varying severity, though neither is very high imho: a) if the system color palette changes when the application is not running (or an svg using such a theme is not running) the cache will not be dropped. this is an existing issue, however, not something introduce by this patchset b) the new system color theme is shared for performance reasons, but once created it is never deleted. it ought to be reference counted so it can be cleaned up after in use, but this is currently not done. several other fixes crept into this patchset (sorry :/) including: * not dropping the cache just because we change themes; this made sense when just plasma-desktop used this or when the apps using Plasma::Theme all used the same theme. this is no longer the case, really, and having applications changing themes randomly kicking the cache out from underneath other apps that may still be using it is undesirable. this does mean that cache files will accumulate. not sure that's a big issue as they don't tend to be large and are in an area marked as cache (so ripe for clean-without-care) * consolidate the signal/slot connections in SvgPrivate::checkColorHints, and now the check is consistent on all paths (previously, it wasn't!) * missing (and possible cause of cache key ambiguity) separator in CACHE_ID_WITH_SIZE * cleanups such as getting rid of superfluous use of Plasma:: namespacing in code that is now in libplasma, such as the stylesheeting. (it was from a plasmoid sebas wrote originally, iirc, explaining the historical reason for the explicit namespacing) in any case, consider this a draft at this stage. i'm interested in geting feedback, improvements and testing. Diffs - /trunk/KDE/kdelibs/plasma/private/svg_p.h 1199366 /trunk/KDE/kdelibs/plasma/svg.cpp 1201684 /trunk/KDE/kdelibs/plasma/theme.cpp 1201685 Diff: http://svn.reviewboard.kde.org/r/6008/diff Testing --- just running normal plasma-desktop and what i use. needs testing with svg's that will actually rely on this feature. Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Supports KDE3 way of starting man/info-pages
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5985/#review9008 --- Ship it! the logic looks fine, but calling it kde3Shortcut is not a great name. it implies knowing what kde3 was doing and doesn't actually describe what the code is attempting to do at all (man/info lookups). please replace it with a descriptive name before committing. - Aaron On 2010-11-27 16:57:02, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5985/ --- (Updated 2010-11-27 16:57:02) Review request for Plasma and Teemu Rytilahti. Summary --- Supports KDE3 of starting man-pages (#term) and info-pages (##term). This is way faster than typing man: or info:. BUG:200556 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/runners/locations/locationrunner.cpp 1200935 Diff: http://svn.reviewboard.kde.org/r/5985/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Kickoff application uninstaller
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5974/ --- (Updated 2010-11-25 23:48:05.415571) Review request for Plasma. Changes --- am part of the plasma group; don't need two copies :) Summary --- Patch to use PackageKit session interface to Uninstall the applications. When kickoff starts it checks if the session interface is available, if it's not available the right click option to Uninstall applications will not be displayed. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 1200785 Diff: http://svn.reviewboard.kde.org/r/5974/diff Testing --- Screenshots --- Kickoff Uninstaller http://svn.reviewboard.kde.org/r/5974/s/563/ Thanks, Daniel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Kickoff application uninstaller
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5974/#review8979 --- looks good (for 4.7), with the exception of using QDBusServiceWatcher. /trunk/KDE/kdebase/workspace/plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp http://svn.reviewboard.kde.org/r/5974/#comment9727 this would be better with a QDBusServiceWatcher (faster, also detect changes at runtime, not have to expose DBus internals directly in the code) - Aaron On 2010-11-25 23:48:05, Daniel Nicoletti wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5974/ --- (Updated 2010-11-25 23:48:05) Review request for Plasma. Summary --- Patch to use PackageKit session interface to Uninstall the applications. When kickoff starts it checks if the session interface is available, if it's not available the right click option to Uninstall applications will not be displayed. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 1200785 Diff: http://svn.reviewboard.kde.org/r/5974/diff Testing --- Screenshots --- Kickoff Uninstaller http://svn.reviewboard.kde.org/r/5974/s/563/ Thanks, Daniel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Kickoff application uninstaller
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5974/#review8981 --- Ship it! a comment in the code explaining why an activatable is being checked for would be nice. - Aaron On 2010-11-25 23:48:05, Daniel Nicoletti wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5974/ --- (Updated 2010-11-25 23:48:05) Review request for Plasma. Summary --- Patch to use PackageKit session interface to Uninstall the applications. When kickoff starts it checks if the session interface is available, if it's not available the right click option to Uninstall applications will not be displayed. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 1200785 Diff: http://svn.reviewboard.kde.org/r/5974/diff Testing --- Screenshots --- Kickoff Uninstaller http://svn.reviewboard.kde.org/r/5974/s/563/ Thanks, Daniel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fixes the run as different user-option in the Shell runner
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5887/#review8783 --- Ship it! - Aaron On 2010-11-17 15:22:17, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5887/ --- (Updated 2010-11-17 15:22:17) Review request for Plasma and Aaron Seigo. Summary --- The run as different user command works again now by using KDESu::SuProcess instead of KDESu::KDEsuClient. The later's behavior was hard to understand, like which pw to enter and how long they will be kept, now the pw will be only kept for this command and has to be always reentered. Further improvements: *The input-fields look different when deactivated. *Also automatically changes the focus to the username-input-field if use as a different user is enabled. BUG:205229 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/runners/shell/shell_config.cpp 1198064 /trunk/KDE/kdebase/workspace/plasma/generic/runners/shell/shellrunner.cpp 1198064 Diff: http://svn.reviewboard.kde.org/r/5887/diff Testing --- Runnin konsole as different user. Tested passwd -S to run in a terminal and as different user, the correct information was printed out. Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Possible improvement for plasma-desktop on multihead by pretending to only have one screen
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5895/ --- Review request for Plasma. Summary --- With this patch, plasma-desktop advertises that it has only one screen when in a multihead environment. It pretends this screen is screen 0 and translates that to the actual screen it is one which it can then use with, e.g., Kephal. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/shell/desktopcorona.cpp 1193327 Diff: http://svn.reviewboard.kde.org/r/5895/diff Testing --- So far: none, as I don't have a multihead system. Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [4.7] Makes it possible to specify the language spell check runner should use, e.g. spell_de $TERM.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5849/#review8736 --- is there a need for the underscore, or for being able to configure it? would it be enough to just check if there is more than one word after spell, check if that's a valid dictionary and if so use it? that would lead to imho the more natural grammar of spell hello, spell en hello, spell de Kraul. being able to write spell german Kraul would be really nice (prevents the user from having to know the language codes) but perhaps asking too much from the runner :) - Aaron On 2010-11-15 12:45:41, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5849/ --- (Updated 2010-11-15 12:45:41) Review request for Plasma and Ryan Bitanga. Summary --- Feature for 4.7 As the summary says, this patch allows to define which dictionary the spell check runner should use. spell will always use the default one. spell_de or more specifically spell_de_DE would work now. Diffs - /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.h 1197278 /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.cpp 1197278 /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck_config.h 1197278 /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck_config.cpp 1197278 /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck_config.ui 1197278 Diff: http://svn.reviewboard.kde.org/r/5849/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [4.7] Makes it possible to specify the language spell check runner should use, e.g. spell_de $TERM.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5849/#review8744 --- Ship it! looks good; the natural language name for the language was just an idea, i didn't expect it to actually be so easy to implement basic support for it. nicely done! :) i'd like to see this in 4.6, tbh. yes, soft feature freeze and all but i don't see the point of waiting until middle of next year for something like this which is small and an obvious benefit by improving an existing feature. - Aaron On 2010-11-15 16:46:01, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5849/ --- (Updated 2010-11-15 16:46:01) Review request for Plasma and Ryan Bitanga. Summary --- Makes it possible to specify the language spell checker should use, e.g. spell $LANG_CODE $TERM or spell $LANG_NAME $TERM. $LANG_CODE could be de, de_AT etc. and $LANG_NAME is the localized language name like german on a english locale or allemand on a french one. Also language names like american english are possible. $LANG_NAME is case insensitive. Diffs - /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.h 1197278 /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.cpp 1197278 Diff: http://svn.reviewboard.kde.org/r/5849/diff Testing --- Tested with both English and German locale set. Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: KRunner: Only one simultanous timer event
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5844/#review8722 --- Ship it! good catch. /trunk/KDE/kdebase/workspace/krunner/krunnerdialog.h http://svn.reviewboard.kde.org/r/5844/#comment9271 may as well keep with the bools above and explicitly set it to be part of the bit field with : 1; - Aaron On 2010-11-14 11:20:53, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5844/ --- (Updated 2010-11-14 11:20:53) Review request for Plasma and Aaron Seigo. Summary --- This patch makes that there is only one timer at once looking for the mouse-cursor position. Diffs - /trunk/KDE/kdebase/workspace/krunner/krunnerdialog.h 1196906 /trunk/KDE/kdebase/workspace/krunner/krunnerdialog.cpp 1196906 Diff: http://svn.reviewboard.kde.org/r/5844/diff Testing --- Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Enables KRunner resizing when it is floating and improves resizing started on the left
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5846/#review8723 --- Ship it! - Aaron On 2010-11-14 11:30:18, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5846/ --- (Updated 2010-11-14 11:30:18) Review request for Plasma and Aaron Seigo. Summary --- KRunner's width can be resized when it is floating. Also resizing on the left size is improved -- the cursor stays at the left edge. BUG:256334 What is still not ideal is when you resize the runner on the left side and it reaches the right screenboarder, then it looks a little choppy but works. There is also still a problem existing -- unrelated to this patch: When it is on top and in the middle and you resize it to the maximum, hide it, try to move it ... it sometimes happens that you can't resize it anymore. Note: This review also includes http://reviewboard.kde.org/r/5844/ as I was not able to do it without via post-review. Diffs - /trunk/KDE/kdebase/workspace/krunner/krunnerdialog.h 1196906 /trunk/KDE/kdebase/workspace/krunner/krunnerdialog.cpp 1196906 Diff: http://svn.reviewboard.kde.org/r/5846/diff Testing --- Tried different cases both in floating and non-floating: resizing it both on the left and right when it is centered, positioned on the left or on the right Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Spell checker runner displays a warning if no valid dictornary could be used
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5847/#review8727 --- Ship it! small whitespace fix needed, but otherwise looks good. /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.cpp http://svn.reviewboard.kde.org/r/5847/#comment9283 fix indentation - Aaron On 2010-11-15 00:07:55, Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5847/ --- (Updated 2010-11-15 00:07:55) Review request for Plasma and Ryan Bitanga. Summary --- If no valid dictionary is specified the spell checker runner will always display that the word is correct. This patch changes it. BUG:245453 NOTE: This patch adds two new i18n strings, so I am not sure if this should go into 4.6 or rather 4.7. Diffs - /trunk/KDE/kdeplasma-addons/runners/spellchecker/spellcheck.cpp 1196911 Diff: http://svn.reviewboard.kde.org/r/5847/diff Testing --- Tried without any spelling dictionaries installed and the warning was displayed. If dictionaries are installed the warning will not be displayed after restarting krunner. Thanks, Matthias ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasma: Folder View: Icon displays 'pressed' on non-activating clicks
On 2010-11-09 18:37:25, Aaron Seigo wrote: Lindsay Roberts wrote: If there are no objections could I request that someone check this in? Markus Slopianka wrote: Does it change the behavior described in https://bugs.kde.org/show_bug.cgi?id=256465 (always using the large 256x256 icon for the scaling effect)? If no, I suggest to tweak the patch because ever since Nuno changed the hi-res icons to look completely different from the smaller ones, the scaling effect looks really weird. Lindsay Roberts wrote: I've looked a bit into this issue: This is at a much lower level than the FolderView itself, it is traceable to KIcon/KIconLoader/KTheme and the strategy used to generate icon's for a given size. This is not a part of, or unique to, Plasma. To visualise this: open Dolphin and navigate to a folder full of movies that have a generic icon, turn previews off, and scale through the available zoom levels. You will see the icon alternating between the smaller icon (on sizes exactly matching the /share/icons/style/SIZExSIZE) and the 256x256 icon (on all other sizes). Having said that, its questionable whether FolderView should be requesting an Icon in this way; what we really want in this instance is (existing_icon * 0.9) not (most_suitable_icon_for(size * 0.9)). But. The behaviour of the icon sizer seems quite clearly incorrect. If we mean to use the largest icon for scaling in all scaled cases we may as well not ship anything but 256x icons, we are essentially not allowing the content disparity the system pertains to provide. Admittedly it is utilised far less than it perhaps could be, but that is no reason not to support it. We could fix the issue by scaling the pixmap from the icon in FolderView itself. There are some issues with regards to whether we cache those scaled pixmaps. Another question: is it wise to cache pixmaps for actions that only happen once on activate and then possibly not for a stretch of time? Perhaps it would be better to leave the cache for icon content that will be regularly repainted. In that case we can just scale on paint() when pressed and delete the pixmap on unpressed or similar. If we don't want to fix it in that way, then there are issues to be dealt with at the KDELibs level, and that would be firmly out of the scope of this patch/review. Even the paint fix might be out of scope of this particular patch, I'll defer to the list on that. i completely agree with this: what we really want in this instance is (existing_icon * 0.9) not (most_suitable_icon_for(size * 0.9)) and that's easy enough to do just by changing the icon requested in the code and then doing the scaling in folderview. in any case, i'll commit this patch as it fixes something regardless of that other behaviour. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5805/#review8589 --- On 2010-11-09 11:50:55, Lindsay Roberts wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5805/ --- (Updated 2010-11-09 11:50:55) Review request for Plasma. Summary --- The folder view employs a 'pressed' effect to show when an icon is activated. This works as expected when global settings are set to single click: the icon is scaled to 90% on press, and rescales back up on activate. Under double click the icon scales on the first click, and weirdly stays scaled until the mouse leaves the hover area. Further, the same issue applies when modifier keys are used to change selection. This addresses bug 256297. https://bugs.kde.org/show_bug.cgi?id=256297 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1193797 Diff: http://svn.reviewboard.kde.org/r/5805/diff Testing --- Tested with both single and double click settings. Thanks, Lindsay ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasma doesn't follow Kiosk restrictions
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5823/#review8680 --- the patch looks incorrect; there are also so many different places in which kiosk settings can be put, that this comment: Plasma doesn't seem to follow Kiosk restrictions at all is vague. which restrictions were tested and didn't work? [$i] at the global scope in plasma-desktop-appletsrc? on a containment group? etc... - Aaron On 2010-11-10 16:39:31, Lubos Lunak wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5823/ --- (Updated 2010-11-10 16:39:31) Review request for Plasma and Marco Martin. Summary --- Probably as a result of r800724, Plasma doesn't seem to follow Kiosk restrictions at all (just grep the sources for where SystemImmutable is assigned to something - it isn't). This patch may possibly be a suitable fix. Diffs - trunk/KDE/kdelibs/plasma/applet.cpp 1182745 Diff: http://svn.reviewboard.kde.org/r/5823/diff Testing --- Thanks, Lubos ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasma doesn't follow Kiosk restrictions
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5823/#review8681 --- please update to include revisions 1196286 and 1196287 and test if that addresses the issues you were seeing. if they don't, please provide a way to reproduce to plasma-devel@kde.org so we can fix it immediately (or, on bugs.kde.org, where it hopefully won't get lost in the noise as that's a rather high traffic area for plasma these days) - Aaron On 2010-11-10 16:39:31, Lubos Lunak wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5823/ --- (Updated 2010-11-10 16:39:31) Review request for Plasma and Marco Martin. Summary --- Probably as a result of r800724, Plasma doesn't seem to follow Kiosk restrictions at all (just grep the sources for where SystemImmutable is assigned to something - it isn't). This patch may possibly be a suitable fix. Diffs - trunk/KDE/kdelibs/plasma/applet.cpp 1182745 Diff: http://svn.reviewboard.kde.org/r/5823/diff Testing --- Thanks, Lubos ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasma: Folder View: Icon displays 'pressed' on non-activating clicks
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5805/#review8589 --- Ship it! - Aaron On 2010-11-09 11:50:55, Lindsay Roberts wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5805/ --- (Updated 2010-11-09 11:50:55) Review request for Plasma. Summary --- The folder view employs a 'pressed' effect to show when an icon is activated. This works as expected when global settings are set to single click: the icon is scaled to 90% on press, and rescales back up on activate. Under double click the icon scales on the first click, and weirdly stays scaled until the mouse leaves the hover area. Further, the same issue applies when modifier keys are used to change selection. This addresses bug 256297. https://bugs.kde.org/show_bug.cgi?id=256297 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1193797 Diff: http://svn.reviewboard.kde.org/r/5805/diff Testing --- Tested with both single and double click settings. Thanks, Lindsay ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Launchersupport in libtaskmanager - final implementation
On 2010-11-07 18:07:13, Aaron Seigo wrote: trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h, lines 129-147 http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40714#file40714line129 with these changes, GroupManager gets a lot of API for launchers. similar API does not exist for windows, startups or groups, though. this seems like the wrong place for such API. why is it added here? if anything, it may make more sense to move addLauncher from GroupManager to TaskGroup. this will allow GroupManager to remain a manager of groups and TaskGroup can continue to manage collections of items. Anton Kreuzkamp wrote: It is possible that you have more than one root group (for every desktop there is one root group), but launchers need to be global, so you can't put in in TaskGroup. (I just see that the launcher gets added to just one rootgroup although, I'll change that) It is possible that you have more than one root group ah, right, due to the show-only-this-desktop thing. *sigh* ok, makes sense. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5785/#review8541 --- On 2010-11-07 13:19:14, Anton Kreuzkamp wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5785/ --- (Updated 2010-11-07 13:19:14) Review request for Plasma. Summary --- This is the final implementation of the launchersupport for libtaskmanager. Many parts of the initial implementation has been changed but now everything works as it should. Diffs - trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/desktopsortingstrategy.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions_p.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1193833 trunk/KDE/kdebase/workspace/libs/taskmanager/taskmanager.h 1193833 Diff: http://svn.reviewboard.kde.org/r/5785/diff Testing --- Tested and everything worked fine. Thanks, Anton ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Launchersupport in libtaskmanager - final implementation
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5785/#review8541
---
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
http://svn.reviewboard.kde.org/r/5785/#comment8907
with these changes, GroupManager gets a lot of API for launchers. similar
API does not exist for windows, startups or groups, though. this seems like the
wrong place for such API. why is it added here?
if anything, it may make more sense to move addLauncher from GroupManager
to TaskGroup. this will allow GroupManager to remain a manager of groups and
TaskGroup can continue to manage collections of items.
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
http://svn.reviewboard.kde.org/r/5785/#comment8906
hasFittingLauncher - hasMatchingLauncher? or better yet, perhaps:
LauncherItem *findLauncher(const AbstractGroupableItem *item) const;
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
http://svn.reviewboard.kde.org/r/5785/#comment8916
are these signals ever used at all? i don't see them used here or in the
tasks plasmoid patch. if they aren't, they should be dropped until there is a
use case for them.
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
http://svn.reviewboard.kde.org/r/5785/#comment8909
odd that is returns NULL even though a launcher exists for it?
it also leaks the launcher created above. instead, i might write it like
this:
LauncherItem *launcher = findLauncher(launcher);
if (!launcher) {
launcher = new LauncherItem ...
}
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
http://svn.reviewboard.kde.org/r/5785/#comment8915
as this is only used from the context menu action, this should be moved
there. the actions are not public API while this is.
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
http://svn.reviewboard.kde.org/r/5785/#comment8910
whitespace
trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp
http://svn.reviewboard.kde.org/r/5785/#comment8913
why Pin Task? that doesn't really tell me what it does at all. what it
really does is create a launcher for the item.
this seems like a very odd place to put the feature. i'd actually expect
the ability to add a launcher using drag and drop from, e.g. a launcher menu or
even through a UI similar to the quicklaunch applet.
trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp
http://svn.reviewboard.kde.org/r/5785/#comment8914
the fact that his can fail hints that this is a feature in the wrong place.
if it is decided that this context menu should be kept (and i'm not yet
convinced it should be), it should _only_ be shown for entries for which a
launcher can be created in the first place.
- Aaron
On 2010-11-07 13:19:14, Anton Kreuzkamp wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5785/
---
(Updated 2010-11-07 13:19:14)
Review request for Plasma.
Summary
---
This is the final implementation of the launchersupport for libtaskmanager.
Many parts of the initial implementation has been changed but now everything
works as it should.
Diffs
-
trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/desktopsortingstrategy.cpp
1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions_p.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1193833
trunk/KDE/kdebase/workspace/libs/taskmanager/taskmanager.h 1193833
Diff: http://svn.reviewboard.kde.org/r/5785/diff
Testing
---
Tested and everything worked fine.
Thanks,
Anton
Re: Review Request: Tasks applet: Make order independent of whether the row count is forced.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/#review8522 --- it's probably that way so that if you say two rows then you get two rows even there are only two buttons. otherwise you'll get a whole row of buttons that takes up just the top row before filling in the rows below, leaving a large section of the widget empty until the rows are filled in with entries. [ window ] [ window ] vs [ window ] [ window ] empty on the other hand, trying to keep a stable # of rows while distributing between the rows would result in entries moving around quite a bit as windows are added .. which also isn't good. could you take some screenshots of the tasks widget running with your patch with different numbers of rows and windows showing in it, that we can use to get some responses to? - Aaron On 2010-11-06 13:09:53, Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/ --- (Updated 2010-11-06 13:09:53) Review request for Plasma. Summary --- This should fix https://bugs.kde.org/show_bug.cgi?id=215231, but frankly I don't understand why it was done this way in the first place... This addresses bug 215231. https://bugs.kde.org/show_bug.cgi?id=215231 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp 1190987 Diff: http://svn.reviewboard.kde.org/r/5776/diff Testing --- Thanks, Ingomar ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Tasks applet: Make order independent of whether the row count is forced.
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/#review8529 --- Ship it! looks good; thanks for the further explanation. i missed that the col/row counts were already being computed elsewhere in the code appropriately. - Aaron On 2010-11-06 20:58:47, Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5776/ --- (Updated 2010-11-06 20:58:47) Review request for Plasma. Summary --- This should fix https://bugs.kde.org/show_bug.cgi?id=215231, but frankly I don't understand why it was done this way in the first place... This addresses bug 215231. https://bugs.kde.org/show_bug.cgi?id=215231 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp 1190987 Diff: http://svn.reviewboard.kde.org/r/5776/diff Testing --- Screenshots --- 3 forced rows, 1-5 windows, trunk vs. patched http://svn.reviewboard.kde.org/r/5776/s/550/ Thanks, Ingomar ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Another approach for PackageStructure for multiple paths per entry
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5765/#review8505 --- i don't see why this patch removes the ability to have multiple paths per entry, since both approaches are useful though perhaps in different situations. the only downside i see to keeping searchPaths for entries is that we don't have an immediate use case for them, which makes it a slightly speculative feature if it isn't used here. i can imagine various situations where it would make sense, however. /trunk/KDE/kdelibs/plasma/package.cpp http://svn.reviewboard.kde.org/r/5765/#comment8860 i don't think this will work: if it truly is fallback based, then it should be just fine to have a required item in any of the context prefixes. /trunk/KDE/kdelibs/plasma/package.cpp http://svn.reviewboard.kde.org/r/5765/#comment8861 same as above /trunk/KDE/kdelibs/plasma/package.cpp http://svn.reviewboard.kde.org/r/5765/#comment8862 the implicit assumption here is that fallbacks are listed first. that's probably fair, but should be noted in the apidox for PackageStructure. same situation with my patch, so not your oversight alone :) /trunk/KDE/kdelibs/plasma/package.cpp http://svn.reviewboard.kde.org/r/5765/#comment8863 this means that the metadata file will be added to the hash for each contents prefix, which is incorrect. that should be moved out of the foreach loop - Aaron On 2010-11-04 14:44:46, Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5765/ --- (Updated 2010-11-04 14:44:46) Review request for Plasma and Aaron Seigo. Summary --- in relation to http://svn.reviewboard.kde.org/r/5763/ i still can't really wrap my mind on that approach, i still see it workig more like this: - the list of fallbacks is defined on the prefix - diffrent platforms would have things in different prefixes, to not have reserved words in the contents/ directory - another shell can have a packagerc with different prefix paths (before plasma/packageformats/%1rc a plasma-$app/packageformats/%1rc should be looked for) Diffs - /trunk/KDE/kdelibs/plasma/package.cpp 1193070 /trunk/KDE/kdelibs/plasma/packagestructure.h 1193070 /trunk/KDE/kdelibs/plasma/packagestructure.cpp 1193070 Diff: http://svn.reviewboard.kde.org/r/5765/diff Testing --- i am not sure the hash still works :/ Thanks, Marco ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make custom colours work on the digital clock again
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/#review8506 --- so once again we descend into a situation where we have a configuration dialog with a billion twiddly options in it. i'm really not very impressed by this, to be honest. the layout could perhaps be a bit cleaner, e.g.: Custom font color: [ x ] ( Color button ) Show shadow: [ x ] Custom shadow color: [ x ] ( Color button ) that's probably clearer and is both one less row and one less widget with the same features. Screenshot: The changed configuration http://svn.reviewboard.kde.org//r/5747/#scomment42 when this is unchecked, both the label and the colour combo should be disabled. it's not really clear to me that the checkbox turns off the shadow versus just turns off a custom color for the shadow. Screenshot: The changed configuration http://svn.reviewboard.kde.org//r/5747/#scomment41 shouldn't have a colon at the end of it, and since there are two colors, it should probably be plural (colors), ditto for Use theme color above. - Aaron On 2010-11-04 02:07:24, Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/ --- (Updated 2010-11-04 02:07:24) Review request for Plasma. Summary --- Since the shadow was introduced for the digital clock a few weeks ago, the custom colour setting has been ignored. This re-enables it, and also allows the user to choose a shadow colour. This changes the configuration dialog and introduces a new option, which is why I'm submitting it for review before committing. Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.h 1191270 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp 1191270 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clockConfig.ui 1191270 Diff: http://svn.reviewboard.kde.org/r/5747/diff Testing --- Changing, saving and loading the settings worked in plasmoid-viewer. Screenshots --- The changed configuration http://svn.reviewboard.kde.org/r/5747/s/546/ Thanks, Alex ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make custom colours work on the digital clock again
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/#review8508 --- Ship it! one small change up, and then i guess it's good to go. Screenshot: Updated http://svn.reviewboard.kde.org//r/5747/#scomment43 should also be disabled when the widgets are - Aaron On 2010-11-04 17:57:08, Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/ --- (Updated 2010-11-04 17:57:08) Review request for Plasma. Summary --- Since the shadow was introduced for the digital clock a few weeks ago, the custom colour setting has been ignored. This re-enables it, and also allows the user to choose a shadow colour. This changes the configuration dialog and introduces a new option, which is why I'm submitting it for review before committing. Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.h 1192604 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp 1192604 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clockConfig.ui 1192604 Diff: http://svn.reviewboard.kde.org/r/5747/diff Testing --- Changing, saving and loading the settings worked in plasmoid-viewer. Screenshots --- The changed configuration http://svn.reviewboard.kde.org/r/5747/s/546/ Updated http://svn.reviewboard.kde.org/r/5747/s/549/ Thanks, Alex ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make custom colours work on the digital clock again
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/#review8501 --- can you include a screenshot of the changed config? i also wonder about the value of making the shadow optional (other than because we can). p.s. it's a little unfortunate that this review mixes both a bug fix and a new feature. - Aaron On 2010-11-01 18:42:29, Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5747/ --- (Updated 2010-11-01 18:42:29) Review request for Plasma. Summary --- Since the shadow was introduced for the digital clock a few weeks ago, the custom colour setting has been ignored. This re-enables it, and also allows the user to choose a shadow colour. This changes the configuration dialog and introduces a new option, which is why I'm submitting it for review before committing. Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.h 1191270 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp 1191270 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clockConfig.ui 1191270 Diff: http://svn.reviewboard.kde.org/r/5747/diff Testing --- Changing, saving and loading the settings worked in plasmoid-viewer. Thanks, Alex ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Group to hash tag conversion in KDE Microblog plasmoid
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5749/#review8469 --- Ship it! trunk/KDE/kdeplasma-addons/applets/microblog/postwidget.cpp http://svn.reviewboard.kde.org/r/5749/#comment8836 forwardText could be const - Aaron On 2010-11-01 21:51:08, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5749/ --- (Updated 2010-11-01 21:51:08) Review request for Plasma. Summary --- Re-denting a post which contains a group-tag results in duplicate posts to the group. This is a major annoyance ( its even mentioned here http://status.net/wiki/Identiquette ). Because people can often forget to do the conversion, let the software do it :) Diffs - trunk/KDE/kdeplasma-addons/applets/microblog/postwidget.cpp 1191412 Diff: http://svn.reviewboard.kde.org/r/5749/diff Testing --- Works for all groups I could test. The matching is performed by using the regex \w, which matches a word. I'm not sure it matches words with numbers too, but at the same time, all the groups I had the patience to check were composed of alphabetical characters. If anyone has some info on this, It'd be nice. Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Added System Wallpaper and My Downloaded Wallpaper checkboxes to sildeshow config
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5705/#review8425 --- i like the idea a lot. there are two issues that must be addressed before it can be committed, however. one is quite trivial: put curly braces around the one liner if statements, kdelibs style. e.g.: the other is almost as trivial: the new checkboxes need the labels fixed. they also need to be put into the layout properly like the rest of the widgets: a QLabel on right with the text, checkbox (with empty text!) on the right. all the widgets should line up from top to bottom along the center line of the label-widget margin. with those fixes, this can go in. thanks for the patch :) /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp http://svn.reviewboard.kde.org/r/5705/#comment8739 use braces, even on single line statements. /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui http://svn.reviewboard.kde.org/r/5705/#comment8741 should be plural and use sentence capitalization: System wallpaper /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui http://svn.reviewboard.kde.org/r/5705/#comment8740 should be plural, and use sentence capitalization: My downloaded wallpapers - Aaron On 2010-10-28 15:28:16, Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5705/ --- (Updated 2010-10-28 15:28:16) Review request for Plasma and Davide Bettio. Summary --- Prompted by bug https://bugs.kde.org/show_bug.cgi?id=253360 and my own itch at not having the ability to easly add ~/.kde/share/wallpaper/ to the slideshow, I added these two checkboxes from this wishlist bug. Let me know if you think the tooltips should be reworded I just took an initial stab at them. Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.h 1190556 /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp 1189844 /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui 1189844 Diff: http://svn.reviewboard.kde.org/r/5705/diff Testing --- I works quite well actually. I still think we need to disable the apply and ok buttons when no checkbox is checked and no paths are in the listbox, but we work around that by putting the system folder in the list if there are no folders in the kconfig. Thanks, Jeremy ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: kdelibs: default-values in config files should be able to handle code-generated default values
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5525/#review7956 --- beyond Marco's observation that we can't add a new virtual to that class (breaks binary compat), i'd honestly rather not support code entries unless we have some good use cases for it, since is likely to be non-trivial to get right. i don't like mingling the ScriptEngine class in with the ConfigLoader API. they are really two completely different things (not to mention how it leads to a fairly ugly set of constructors :). at the very least it should be up the owner of the ConfigLoader class to call setScriptEngine, but even then, i'm not sure that'll work. if we just look at the JavaScript case, the plasmoid does not really control when the config file is loaded. which means that the value can be non-deterministic unless it's a simple bit of code that is fully self contained (e.g. doesn't reference any other functions, variables, etc in the plasmoid itself). this is because when evaluate is called on QSCriptEngine the code is run in the current context. which can be almost anything, since activeConfig can be set pretty much whenever. probably the code should run in the top-level context for the package. remember that we now have addons, which means they could have their own kconfigxt files, and those should be evaluated in the context that the addon was created in. thankfully we can actually know what that context is these days since each are marked with the package as a hidden property. not to mention issues of keeping the code that is evaluated() from accidentally stomping on, e.g., local variables. (that's easy enough to avoid though, with a new context created for it to be eval'd within. (on a side note, i just noticed that settings files aren't kept separate for addons and the main applet, which means they can rather easily get jumbled. i'll have to come up with a solution for that one, likely by keeping each set of config objects separate for the main plasmoid and for each addon. blarg.) this means, however, that where ConfigLoader calls evaluate, it also needs to be passing in more information that just here, execute this script. the API will rapidly get messy. next, from a principle-of-least-surprise perspective, i'd expect this code to be live when writing javascript. when it's used with C++ it isn't - it is executated exactly once on construction of the settings class. but in those cases, the creation of the KConfigSkeleton object is controlled by the code using it. in this case, the plasmoid doesn't know when that happens and currently doesn't have a way to say ok, put it away and re-parse it. so in the C++ case, it is really just run it once; it's run it each time the settings object is created. emulating that rather useful behaviour in JS is going to tricky, if only because we will need to decide whether to unload the settings every time activeConfig is set as a way to emulate the C++ behaviour (which is partly what makes code=true so useful in the first place), explicit rules for when the main config is parsed (though that's simple enough: before the first value is called on it), etc. the alternative is to make the code execute whenever the default is requested. this would easy enough with QScriptEngine: each default that is code could be wrapped up in an anonymous function in the correct context and the QScriptValue that results would be held onto. whenever the default would be needed, that QScriptValue would get call()'d, and the returned value fed right back into the script env. as a bonus - this would completely get rid of the problems with objects and constructors being passed back and forth since it would never actually leave the scripting env. what might make sense it to allow configLoader to parse code=true items (again, assuming we have good use cases for it :) and then be able to return a list of items that have code associated with them. then in the JS ScriptEngine, we could make up a small subclass of ConfigLoader which, after the ConfigLoader is done parsing, can ask for the list of all items with code for defaults and set up the anonymous functions for each. all that would remain is knowing when the generate the default; this could be accomplished by setting a default constructed object (e.g. QString(), QColor(), etc) as the default in such cases, and then it would need to run the anon function if property() is the default. this would mean a small amount of extra overhead on reading values - check if it has code associated with it, if so, then get property() and see if it is the default (could be a convenience method in ConfigLoader for that; i don't think KConfigSkeletonItem makes that very easy on its own; or ConfigLoader::property() could just return QVariant() in those cases
Re: Review Request: kdelibs: default-values in config files should be able to handle code-generated default values
On 2010-10-04 04:41:13, Aaron Seigo wrote: beyond Marco's observation that we can't add a new virtual to that class (breaks binary compat), i'd honestly rather not support code entries unless we have some good use cases for it, since is likely to be non-trivial to get right. i don't like mingling the ScriptEngine class in with the ConfigLoader API. they are really two completely different things (not to mention how it leads to a fairly ugly set of constructors :). at the very least it should be up the owner of the ConfigLoader class to call setScriptEngine, but even then, i'm not sure that'll work. if we just look at the JavaScript case, the plasmoid does not really control when the config file is loaded. which means that the value can be non-deterministic unless it's a simple bit of code that is fully self contained (e.g. doesn't reference any other functions, variables, etc in the plasmoid itself). this is because when evaluate is called on QSCriptEngine the code is run in the current context. which can be almost anything, since activeConfig can be set pretty much whenever. probably the code should run in the top-level context for the package. remember that we now have addons, which means they could have their own kconfigxt files, and those should be evaluated in the context that the addon was created in. thankfully we can actually know what that context is these days since each are marked with the package as a hidden property. not to mention issues of keeping the code that is evaluated() from accidentally stomping on, e.g., local variables. (that's easy enough to avoid though, with a new context created for it to be eval'd within. (on a side note, i just noticed that settings files aren't kept separate for addons and the main applet, which means they can rather easily get jumbled. i'll have to come up with a solution for that one, likely by keeping each set of config objects separate for the main plasmoid and for each addon. blarg.) this means, however, that where ConfigLoader calls evaluate, it also needs to be passing in more information that just here, execute this script. the API will rapidly get messy. next, from a principle-of-least-surprise perspective, i'd expect this code to be live when writing javascript. when it's used with C++ it isn't - it is executated exactly once on construction of the settings class. but in those cases, the creation of the KConfigSkeleton object is controlled by the code using it. in this case, the plasmoid doesn't know when that happens and currently doesn't have a way to say ok, put it away and re-parse it. so in the C++ case, it is really just run it once; it's run it each time the settings object is created. emulating that rather useful behaviour in JS is going to tricky, if only because we will need to decide whether to unload the settings every time activeConfig is set as a way to emulate the C++ behaviour (which is partly what makes code=true so useful in the first place), explicit rules for when the main config is parsed (though that's simple enough: before the first value is called on it), etc. the alternative is to make the code execute whenever the default is requested. this would easy enough with QScriptEngine: each default that is code could be wrapped up in an anonymous function in the correct context and the QScriptValue that results would be held onto. whenever the default would be needed, that QScriptValue would get call()'d, and the returned value fed right back into the script env. as a bonus - this would completely get rid of the problems with objects and constructors being passed back and forth since it would never actually leave the scripting env. what might make sense it to allow configLoader to parse code=true items (again, assuming we have good use cases for it :) and then be able to return a list of items that have code associated with them. then in the JS ScriptEngine, we could make up a small subclass of ConfigLoader which, after the ConfigLoader is done parsing, can ask for the list of all items with code for defaults and set up the anonymous functions for each. all that would remain is knowing when the generate the default; this could be accomplished by setting a default constructed object (e.g. QString(), QColor(), etc) as the default in such cases, and then it would need to run the anon function if property() is the default. this would mean a small amount of extra overhead on reading values - check if it has code associated with it, if so, then get property() and see if it is the default (could be a convenience method in ConfigLoader for that; i don't think KConfigSkeletonItem makes that very easy on its own; or ConfigLoader::property() could just return QVariant() in those cases prevening another external
Re: Review Request: use sqlite for storage
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5506/#review7926 --- the implementation looks good; my one question is regards keeping the data from different applets separate. /trunk/KDE/kdelibs/plasma/private/storage.cpp http://svn.reviewboard.kde.org/r/5506/#comment8176 instead of an incrementing connectionId, you could also use the value of the this pointer? one less static int kicking around. /trunk/KDE/kdelibs/plasma/private/storage.cpp http://svn.reviewboard.kde.org/r/5506/#comment8178 destination() is no longer used; with one db file used for all the storage, we end up with all the data in one db with no separation. it would make sense to me here to create a table for each destination(). on applet destruction, it could drop its table (if any). on applet export, it could pull its table over to an exported db. (i imagine that Corona would use this in its export method). /trunk/KDE/kdelibs/plasma/private/storage.cpp http://svn.reviewboard.kde.org/r/5506/#comment8179 ah, sql databases. this query will result in multiple rows with the same key, and when requested back you'll end up with a random row. to fix this, first delete the old row. or check if the old row exists and do an update instead of an insert. and no, there is no update or insert in sql. that's what sorted procedures are for ;P - Aaron On 2010-10-01 15:05:11, Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5506/ --- (Updated 2010-10-01 15:05:11) Review request for Plasma. Summary --- looking at how slow kconfig as, this makes storage use sqlite (and makes some methods private, before it's too late) it can still use some improvements but it's basically working. two main concerns are: - is acceptable/safe to link to QtSql and assume the sqlite driver is present? - i would still see this as a fallback for when an akonadi version is not present (being in another process should slowdown the gui a bit less, but i could not want it in some mobile profiles) the akonadi version is in the usual status almost working with developer disappeared :p Diffs - /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1179394 /trunk/KDE/kdelibs/plasma/datacontainer.h 1179394 /trunk/KDE/kdelibs/plasma/datacontainer.cpp 1179394 /trunk/KDE/kdelibs/plasma/dataengine.cpp 1179394 /trunk/KDE/kdelibs/plasma/private/datacontainer_p.h 1179394 /trunk/KDE/kdelibs/plasma/private/storage.cpp 1179394 /trunk/KDE/kdelibs/plasma/private/storage_p.h 1179394 Diff: http://svn.reviewboard.kde.org/r/5506/diff Testing --- Thanks, Marco ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fix PaintUtils::shadowText() placement
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5504/#review7927 --- Ship it! - Aaron On 2010-10-01 02:00:06, Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5504/ --- (Updated 2010-10-01 02:00:06) Review request for Plasma. Summary --- I used Plasma::PaintUtils::shadowText() to draw a text halo (offset 0,0) around a text. Unfortunately, that halo got cropped (see the g). This patch should * fix placement of shadow and text inside the resulting pixmap * fix addSize not enlarged for negative offsets * simplify computation Diffs - /trunk/KDE/kdelibs/plasma/paintutils.cpp 1181424 Diff: http://svn.reviewboard.kde.org/r/5504/diff Testing --- Screenshots --- before http://svn.reviewboard.kde.org/r/5504/s/517/ after http://svn.reviewboard.kde.org/r/5504/s/519/ Thanks, Christoph ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: bug fixes for the system-monitor applet
On 2010-09-25 08:31:36, Petri Damstén wrote: Looks good to me. has it been committed? if not, can you please do so, and mark this as submitted. thanks. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/3950/#review7773 --- On 2010-05-14 08:21:03, Michel Lafon-Puyo wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/3950/ --- (Updated 2010-05-14 08:21:03) Review request for Plasma. Summary --- As my first work for KDE, I would like to add some power management monitoring features to the system-monitor applet. Monitoring the cpu clock frequency when using the on-demand or the conservative governor could be useful (at least it is for me :)). Before going further in this development I tried to fix some little bugs with the applet. Here is the list of the changes this patch introduces: - the size of the applets when used in the panel and when no item is monitored was (0,0) but an icon were displayed and it overlapped the other applets (SM::Applet::CheckGeometry()) - set the preferred height to MINIMUM when no item is monitored (SM::Applet::displayNoAvailableSources()). When all the meters of an applet were removed at once, the size were unchanged and a big icon could be displayed. - clear the content of the tooltip when nothing has to be displayed (SM::Applet::toolTipAboutToShow()) - when one or many items were set unmonitored, the corresponding widgets were not correctly deleted (SM::Applet::deleteMeters()) - the removal of the layout and the meters were done on SM::Applet::connectToEngine(), I moved that part in a new method SM::Applet::removeLayout() that can be called more easily. This method is called by the applets on configuration change to achieve a clean update. - the header of the applets is now correctly deleted on form factor change and should not be displayed when the applet is used in the panel - when used in the panel, the webview containing the information given by the hardware information applet was displayed under the icon because the webview and the icon were not correctly deleted on form factor changes. - to be consistent with the other applets, the HDD applet has been changed to *not* populate the configuration with the list of the mounted volumes when there is no more item to monitor. Nevertheless, on the first launch (no configuration is present), the behaviour has not changed and the configuration is still populated with the list of the mounted volumes. Additionnally, the HDD applet didn't use the SM::Applet to manage its meters. So I replaced the list of SM::Plotter (m_plotters) by a list of QGraphicsWidget (m_meters) and modified HDD to take advantage of the meters management already implemented in the SM::Applet class (in particular the removal of the meters/widgets on configuration change as mentioned above). Note: I was unable to find any bug reports corresponding to the fixes above. Should I create them myself? Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/applet.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/cpu.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hdd.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/hwinfo.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/net.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/ram.cpp 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.h 1126529 /trunk/KDE/kdebase/workspace/plasma/generic/applets/system-monitor/temperature.cpp 1126529 Diff: http://svn.reviewboard.kde.org/r/3950/diff Testing --- Basic testing Thanks, Michel ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: 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/#review7929 --- Ship it! much nicer. - Aaron On 2010-09-27 10:57:33, Anselmo L. S. Melo wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5446/ --- (Updated 2010-09-27 10:57:33) 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: Use Plasma::ScrollWidget in the widget explorer
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4676/#review7930 --- Ship it! update to current trunk if needed, and let's get this in as well. - Aaron 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
Re: Review Request: Optional passive calendar popup for the Digital Clock
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5513/#review7934 --- as discussed earlier today on irc, and for all the reasons discussed there, this is not a change we want or care to have. it's one more code path that will end up being maintained by yours truly in the future. sorry. you are completely welcome to maintain this as a separate plasmoid or as a patch to the source, of course. The only downside is that you either have to relog or somehow reload that digital-clock plasmoid when you change the passivePopup value. that's probably because you need to read the value in configChanged(). /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp http://svn.reviewboard.kde.org/r/5513/#comment8183 i don't see why the behaviour should be different in other form factors. *shrug* - Aaron On 2010-10-02 02:05:02, Mark wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5513/ --- (Updated 2010-10-02 02:05:02) Review request for Plasma. Summary --- I know some people don't like this idea thus the default is the same as it's right now. I want to make the calendar passive popup optional since some people like it that way (me included). OK to commit? Diffs - /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp 1181669 /trunk/KDE/kdebase/workspace/plasma/generic/applets/digital-clock/clock.cpp 1181669 Diff: http://svn.reviewboard.kde.org/r/5513/diff Testing --- Tested it a few times with true and false and seems to work just fine. The only downside is that you either have to relog or somehow reload that digital-clock plasmoid when you change the passivePopup value. Thanks, Mark ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make kolourpicker color grab working from autohiding panels
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5465/#review7884 --- Ship it! looks good, though the patch looks like it hasn't been tidied up for commit yet. the code itself looks good, though, so just tidy it up and commit. nicely done! /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.h http://svn.reviewboard.kde.org/r/5465/#comment8134 delete the line before committing :) /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp http://svn.reviewboard.kde.org/r/5465/#comment8135 watch the whitespace :) /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp http://svn.reviewboard.kde.org/r/5465/#comment8136 delete before commit - Aaron On 2010-09-28 10:01:19, Björn Ruberg wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5465/ --- (Updated 2010-09-28 10:01:19) Review request for Plasma. Summary --- Patch fixes the bug that the mouse grab of the color-picker goes away when the panel hides. The idea of initiating the grab not on the panel but on a self created widget is from ksnapshot. I want someone to look at it because I want to commit it KDE 4.5 branch as a bugfix. This addresses bug 252350. https://bugs.kde.org/show_bug.cgi?id=252350 Diffs - /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.h 1180054 /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp 1180054 Diff: http://svn.reviewboard.kde.org/r/5465/diff Testing --- Had kolourpicker in an autohiding panel - and it suddenly works. Tested with an applet on the desktop too. Works. Thanks, Björn ___ 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... 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 is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5451/#review7824 --- trunk/KDE/kdelibs/plasma/corona.cpp http://svn.reviewboard.kde.org/r/5451/#comment8094 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? trunk/KDE/kdelibs/plasma/corona.cpp http://svn.reviewboard.kde.org/r/5451/#comment8095 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? - Aaron 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: Cleanups in pager
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/#review7825 --- Ship it! - Aaron On 2010-09-24 23:59:14, Anthony Bryant wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/ --- (Updated 2010-09-24 23:59:14) Review request for Plasma. Summary --- * Make configAccepted() just write the new config, and make configChanged() conditionally update the plasmoid after reading the config back in. * Split the grid resizing logic into its own method: recalculateGridSizes(int rows), and make it detect more edge cases. * Rename the main size calculation method to updateSizes(bool allowResize), which will resize the applet to its preferred size if allowResize is true. * Fix a bug which caused applet resizes to be wrongly cancelled in some situations. * Code style cleanups, remove unused variables, etc. (Sorry for putting so many changes into one patch) This addresses bug 250756. https://bugs.kde.org/show_bug.cgi?id=250756 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.h 1179262 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1179262 Diff: http://svn.reviewboard.kde.org/r/5355/diff Testing --- Tested configuring the pager from the config interface and the desktop console, and that it responds correctly to changes in desktop count, and in FormFactor and size. Thanks, Anthony ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Cleanups in pager
On 2010-09-17 16:18:36, Aaron Seigo wrote: the patch doesn't apply to current trunk .. could you update the review board request? thanks :) Anthony Bryant wrote: I thought I did that yesterday (and there haven't been any commits to pager since then), does it still not apply? Beat Wolf wrote: does this patch solve https://bugs.kde.org/show_bug.cgi?id=250756 ? Anthony Bryant wrote: It does. It should make sure changes to the number of virtual desktops cause a resize of the widget every time, instead of having a 1/2 chance of ignoring them. this patch still does not apply cleanly to trunk. did you edit or open the patch in a text editor, or some such, that messes with whitespace? if you are uploading your patch by hand, please try out the post-review tool: http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/ - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/#review7667 --- On 2010-09-17 02:29:59, Anthony Bryant wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/ --- (Updated 2010-09-17 02:29:59) Review request for Plasma. Summary --- * Make configAccepted() just write the new config, and make configChanged() conditionally update the plasmoid after reading the config back in. * Split the grid resizing logic into its own method: recalculateGridSizes(int rows), and make it detect more edge cases. * Rename the main size calculation method to updateSizes(bool allowResize), which will resize the applet to its preferred size if allowResize is true. * Fix a bug which caused applet resizes to be wrongly cancelled in some situations. * Code style cleanups, remove unused variables, etc. (Sorry for putting so many changes into one patch) This addresses bug 250756. https://bugs.kde.org/show_bug.cgi?id=250756 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.h 1176214 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1176214 Diff: http://svn.reviewboard.kde.org/r/5355/diff Testing --- Tested configuring the pager from the config interface and the desktop console, and that it responds correctly to changes in desktop count, and in FormFactor and size. Thanks, Anthony ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Share FrameData between FrameSvg's
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5416/ --- (Updated 2010-09-23 06:39:30.784225) Review request for Plasma. Summary --- This patch implements shared FrameData between FrameSvg's. It uses a per-FrameSvg count since it is feasible that one FrameSvg could reference the same FrameData more than once. The results are reasonable: ~25% fewer pixmaps in a default plasma-desktop startup, %57 fewer pixmaps with a layout of 4 buttons, 4 line edits and a combobox in a grid layout. This results in fewer xserver roundtrips on x11 and less memory usage on all platforms. There is a #define near the dtor of FrameSvgPrivate that can be uncommented to see the benefits and confirm that all shared pages are indeed removed at the end. Confirming with ktracepixmap was also done. Diffs (updated) - /trunk/KDE/kdelibs/plasma/framesvg.h 1178444 /trunk/KDE/kdelibs/plasma/framesvg.cpp 1178444 /trunk/KDE/kdelibs/plasma/private/framesvg_p.h 1178444 Diff: http://svn.reviewboard.kde.org/r/5416/diff Testing --- Full plasma-desktop session, test plasmoid with the buttons'n'labels torture test (over 100 of each in a grid layout). Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Share FrameData between FrameSvg's
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5416/ --- (Updated 2010-09-23 06:47:25.265223) Review request for Plasma. Summary --- This patch implements shared FrameData between FrameSvg's. It uses a per-FrameSvg count since it is feasible that one FrameSvg could reference the same FrameData more than once. The results are reasonable: ~25% fewer pixmaps in a default plasma-desktop startup, %57 fewer pixmaps with a layout of 4 buttons, 4 line edits and a combobox in a grid layout. This results in fewer xserver roundtrips on x11 and less memory usage on all platforms. There is a #define near the dtor of FrameSvgPrivate that can be uncommented to see the benefits and confirm that all shared pages are indeed removed at the end. Confirming with ktracepixmap was also done. Diffs (updated) - /trunk/KDE/kdelibs/plasma/framesvg.h 1178444 /trunk/KDE/kdelibs/plasma/framesvg.cpp 1178444 /trunk/KDE/kdelibs/plasma/private/framesvg_p.h 1178444 Diff: http://svn.reviewboard.kde.org/r/5416/diff Testing --- Full plasma-desktop session, test plasmoid with the buttons'n'labels torture test (over 100 of each in a grid layout). Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: fewer pixmaps created in pixmap transitions
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5406/ --- Review request for Plasma. Summary --- This prevents the creation of new pixmaps as much as possible while doing transition effects at the cost of creating more pixmaps on the fly. It introduces a new property, cache, to PixmapTransition to restore the old behaviour. However, most of our widgets already do caching internally, so the added pixmap copy is just that: a copy. It sits around taking up pixmap memory for no particularly good reason. In the Javascript plasmoid I'm working on, it creates a table with buttons, some 146 buttons, in fact. This drops the Pixmap count from nearly 150 for the same (pixel-wise) button pixmap to 1. Combined with my commits from last night and a new caching scheme for FrameSvg, creating these buttons has dropped from 6-9 seconds on my machine to 2-3 seconds. Diffs - /trunk/KDE/kdelibs/plasma/animations/pixmaptransition.cpp 1178174 /trunk/KDE/kdelibs/plasma/animations/pixmaptransition_p.h 1178174 /trunk/KDE/kdelibs/plasma/paintutils.cpp 1178174 Diff: http://svn.reviewboard.kde.org/r/5406/diff Testing --- g...@coherenttheory.com:plasmoidprogress.git Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: fewer pixmaps created in pixmap transitions
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5406/ --- (Updated 2010-09-22 18:36:20.575729) Review request for Plasma. Summary --- This prevents the creation of new pixmaps as much as possible while doing transition effects at the cost of creating more pixmaps on the fly. It introduces a new property, cache, to PixmapTransition to restore the old behaviour. However, most of our widgets already do caching internally, so the added pixmap copy is just that: a copy. It sits around taking up pixmap memory for no particularly good reason. In the Javascript plasmoid I'm working on, it creates a table with buttons, some 146 buttons, in fact. This drops the Pixmap count from nearly 150 for the same (pixel-wise) button pixmap to 1. Combined with my commits from last night and a new caching scheme for FrameSvg, creating these buttons has dropped from 6-9 seconds on my machine to 2-3 seconds. Diffs (updated) - /trunk/KDE/kdelibs/plasma/animations/pixmaptransition.cpp 1178174 /trunk/KDE/kdelibs/plasma/animations/pixmaptransition_p.h 1178174 /trunk/KDE/kdelibs/plasma/paintutils.cpp 1178174 Diff: http://svn.reviewboard.kde.org/r/5406/diff Testing --- g...@coherenttheory.com:plasmoidprogress.git Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Share FrameData between FrameSvg's
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5416/ --- Review request for Plasma. Summary --- This patch implements shared FrameData between FrameSvg's. It uses a per-FrameSvg count since it is feasible that one FrameSvg could reference the same FrameData more than once. The results are reasonable: ~25% fewer pixmaps in a default plasma-desktop startup, %57 fewer pixmaps with a layout of 4 buttons, 4 line edits and a combobox in a grid layout. This results in fewer xserver roundtrips on x11 and less memory usage on all platforms. There is a #define near the dtor of FrameSvgPrivate that can be uncommented to see the benefits and confirm that all shared pages are indeed removed at the end. Confirming with ktracepixmap was also done. Diffs - /trunk/KDE/kdelibs/plasma/framesvg.h 1178174 /trunk/KDE/kdelibs/plasma/framesvg.cpp 1178174 /trunk/KDE/kdelibs/plasma/private/framesvg_p.h 1178174 Diff: http://svn.reviewboard.kde.org/r/5416/diff Testing --- Full plasma-desktop session, test plasmoid with the buttons'n'labels torture test (over 100 of each in a grid layout). Thanks, Aaron ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround plasma tooltips causing holes in windows
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5415/#review7725 --- Ship it! this doesn't fix other ways to reproduce this bug, e.g. drop down menus. but this is probably the most common cause. (that it happens when showing a window that was previously shown only is an interesting clue for the x.org people, perhaps.) however, given that this bug has been sitting there unfixed for a couple of _years_ now with no fix in sight and that this is the most prevalent cause, coupled with it not really being a huge hack in libplasma, i'm in favour of this change in spite of the don't work around bugs philosophy. cheers ... - Aaron On 2010-09-22 23:13:41, Anthony Bryant wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5415/ --- (Updated 2010-09-22 23:13:41) Review request for Plasma. Summary --- Works around an X bug that causes plasma tooltips to leave behind weird holes in windows (when compositing is off): If you trigger a tooltip which has a window under it, let the tooltip disappear, and then trigger a tooltip somewhere else, a hole appears in the window where the first tooltip used to be. Basically, this patch deletes the tooltip's window whenever it is hidden, and re-creates it when it is shown. Since it's a different window being shown the next time, the X bug doesn't happen. I know this is a workaround for an X bug, so if you want to discard this patch and see X get fixed instead, then I can sympathize. However, this workaround doesn't introduce any hacks into libplasma, it just deletes a window when it isn't actually in use. Diffs - /trunk/KDE/kdelibs/plasma/tooltipmanager.cpp 1178011 Diff: http://svn.reviewboard.kde.org/r/5415/diff Testing --- Hovered over several plasmoids, and checked that changing the tooltip while it is still visible works as well as deleting and re-creating it. Thanks, Anthony ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: DataEngine: Add methods to allow disabling auto-deletion of data sources created on demand
On 2010-09-10 23:56:04, Aaron Seigo wrote:
if there is nothing connected to it, why would it want to fill that data?
if nothing is connected, then there is precisely no reason to deliver that
data. it sounds like the system monitor engine is doing something wrong.
Alex Merry wrote:
System monitor maintains its list of sources (ie: things that can be
monitored) by maintaining a bunch of DataContainers. Ordinarily, these just
have some meta-information about the thing being monitored, such as a
human-readable name and the units. Only when a source is connected to are
the values updated.
This is not an unusual approach to take, as far as I'm aware - that's
kind of the point of DataContainer, right? It's just that systemmonitor
doesn't bother subclassing DataContainer, but instead uses it directly.
Note that the way KSysGuard works means that if it fetches the list of
things that can be monitored, it has to fetch the meta-info as well, so it
may as well store it. The issue is simply that it has to fetch this
asynchonously, and visualizations may request a source before it has done so.
Aaron Seigo wrote:
This is not an unusual approach to take
no, it is quite unusual. a DataContainer, once added to an engine due to
a sourceRequestEvents not only represents that source but the source has the
lifespan of the request.
instead, what it ought to do is add all the DataContainers using
DataEngine::addSource(DataContainer *) and then update them in
updateRequestEvent. this behaviour can be triggered with
setMinimumPollingInterval(0). this is documented in the apidox and is the way
to get the desired behavior.
so, yes, SystemMonitor is at fault here :)
Alex Merry wrote:
I don't see how that would help.
The issue systemmonitor is trying to deal with is that it finds out what
sources it should add *after* one of them is requested. So it has the option
of refusing to deal with the request at all (in which case visualizations
would need to listen to sourceAdded() - not a problem in itself, but this
make break existing visualizations using systemmonitor) or adding it in the
hope that it will be created (the current behaviour, and the cause of the
problems).
With the latter approach, when it finds out what sources are available,
it has the choice of populating the existing source (in which case it may
well be removed by DataEngine at some point) or removing it and re-adding it
(disconnecting the visualization in the process, defeating the purpose of
creating the dummy source in the first place).
an easier way around this is to disconnect the DataContainer's becameUnused
signal:
DataContainer *s = containerForSource(source);
if (s) {
disconnect(s, SIGNAL(becameUnused(QString))), this,
SLOT(removeSource(QString)));
}
- Aaron
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5312/#review7530
---
On 2010-09-10 23:30:32, Alex Merry wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5312/
---
(Updated 2010-09-10 23:30:32)
Review request for Plasma.
Summary
---
Allow DataEngine implementations to prevent DataEngine from automatically
deleting sources create in sourceRequestEvent.
Rationale: engines that fetch a list of sources asynchronously at creation
time (such as systemmonitor) may reasonably want to create dummy data sources
when they are requested before the list has been fetched. However, they
probably don't want these to be removed again when disconnected from.
Diffs
-
/trunk/KDE/kdelibs/plasma/dataengine.cpp 1173953
/trunk/KDE/kdelibs/plasma/private/dataengine_p.h 1173953
/trunk/KDE/kdelibs/plasma/dataengine.h 1173953
Diff: http://svn.reviewboard.kde.org/r/5312/diff
Testing
---
It makes the systemmonitor engine work properly. Specifically, it solves the
bug in bubblemon where if you had the bubblemon displaying, say, CPU Total
Load when plasma started and then changed it to, say, CPU Idle Load, the CPU
Total Load source would be removed and there would be an empty place where it
should be in the list in the config dialog.
Thanks,
Alex
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Cleanups in pager
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/#review7667 --- the patch doesn't apply to current trunk .. could you update the review board request? thanks :) - Aaron On 2010-09-17 02:29:59, Anthony Bryant wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5355/ --- (Updated 2010-09-17 02:29:59) Review request for Plasma. Summary --- * Make configAccepted() just write the new config, and make configChanged() conditionally update the plasmoid after reading the config back in. * Split the grid resizing logic into its own method: recalculateGridSizes(int rows), and make it detect more edge cases. * Rename the main size calculation method to updateSizes(bool allowResize), which will resize the applet to its preferred size if allowResize is true. * Fix a bug which caused applet resizes to be wrongly cancelled in some situations. * Code style cleanups, remove unused variables, etc. (Sorry for putting so many changes into one patch) This addresses bug 250756. https://bugs.kde.org/show_bug.cgi?id=250756 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.h 1176214 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1176214 Diff: http://svn.reviewboard.kde.org/r/5355/diff Testing --- Tested configuring the pager from the config interface and the desktop console, and that it responds correctly to changes in desktop count, and in FormFactor and size. Thanks, Anthony ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an unit test for Plasma::ConfigLoader
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5329/#review7582 --- Ship it! looks great; and it shows a problem in ConfigLoader - it should be using int, not qint32 for the integer list. nice! :) - Aaron On 2010-09-13 17:51:03, Martin Blumenstingl wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5329/ --- (Updated 2010-09-13 17:51:03) Review request for Plasma. Summary --- Currently I'm changing some of the code in Plasma::ConfigLoader. I want to make sure that I don't break that much ;) Thus I wrote a unit test for the config loader. Currently all it does it testing if the config loader can parse the default-values correctly. One test for each (data-)type which ConfigLoader can handle. Diffs - /trunk/KDE/kdelibs/plasma/tests/configloadertest.xml PRE-CREATION /trunk/KDE/kdelibs/plasma/tests/CMakeLists.txt 1174512 /trunk/KDE/kdelibs/plasma/tests/configloadertest.h PRE-CREATION /trunk/KDE/kdelibs/plasma/tests/configloadertest.cpp PRE-CREATION Diff: http://svn.reviewboard.kde.org/r/5329/diff Testing --- I ran the tests on my box. All of them were successful. Thanks, Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: configChanged() for the Tasks plasmoid
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5313/#review7545 --- Ship it! looks good; another plasmoid down! :) /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp http://svn.reviewboard.kde.org/r/5313/#comment7713 usually put the conditional on the end of the line above, e.g.: if (item-abstractItem() item-abstractItem()-.. - Aaron On 2010-09-11 02:00:33, Anthony Bryant wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5313/ --- (Updated 2010-09-11 02:00:33) Review request for Plasma. Summary --- Add configChanged() to the Tasks plasmoid, and fix a few warnings. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/abstracttaskitem.cpp 1173935 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskgroupitem.cpp 1173935 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp 1173935 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasks.h 1173935 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasks.cpp 1173935 Diff: http://svn.reviewboard.kde.org/r/5313/diff Testing --- Tested that tooltips can be toggled from both the UI and the desktop console, and that the other options still work from the UI. Thanks, Anthony ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: wetter.com Ion: fix forecast retrieval for some geographic regions
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5309/#review7527 --- Ship it! looks like a reasonable fix. please commit. thanks! :) - Aaron On 2010-09-10 17:25:33, Thilo-Alexander Ginkel wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5309/ --- (Updated 2010-09-10 17:25:33) Review request for Plasma and Shawn Starr. Summary --- As it turned out, wetter.com seems to return temperatures as decimal fractions for some geographic regions (such as Turkey) whereas most other regions use integers. Retrieving weather data for these regions unfortunately failed as QString.toInt() does not work with decimal fractions. Example XML data showcasing this is attached to the referenced bug report. This fix corrects the issue by converting the data parsed from the XML to a double and then rounds it to an integer. This addresses bug 236309. https://bugs.kde.org/show_bug.cgi?id=236309 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/dataengines/weather/ions/wetter.com/ion_wettercom.cpp 1173706 Diff: http://svn.reviewboard.kde.org/r/5309/diff Testing --- Thanks, Thilo-Alexander ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: DataEngine: Add methods to allow disabling auto-deletion of data sources created on demand
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5312/#review7530 --- if there is nothing connected to it, why would it want to fill that data? if nothing is connected, then there is precisely no reason to deliver that data. it sounds like the system monitor engine is doing something wrong. - Aaron On 2010-09-10 23:30:32, Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5312/ --- (Updated 2010-09-10 23:30:32) Review request for Plasma. Summary --- Allow DataEngine implementations to prevent DataEngine from automatically deleting sources create in sourceRequestEvent. Rationale: engines that fetch a list of sources asynchronously at creation time (such as systemmonitor) may reasonably want to create dummy data sources when they are requested before the list has been fetched. However, they probably don't want these to be removed again when disconnected from. Diffs - /trunk/KDE/kdelibs/plasma/dataengine.cpp 1173953 /trunk/KDE/kdelibs/plasma/private/dataengine_p.h 1173953 /trunk/KDE/kdelibs/plasma/dataengine.h 1173953 Diff: http://svn.reviewboard.kde.org/r/5312/diff Testing --- It makes the systemmonitor engine work properly. Specifically, it solves the bug in bubblemon where if you had the bubblemon displaying, say, CPU Total Load when plasma started and then changed it to, say, CPU Idle Load, the CPU Total Load source would be removed and there would be an empty place where it should be in the list in the config dialog. Thanks, Alex ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: DataEngine: Add methods to allow disabling auto-deletion of data sources created on demand
On 2010-09-10 23:56:04, Aaron Seigo wrote: if there is nothing connected to it, why would it want to fill that data? if nothing is connected, then there is precisely no reason to deliver that data. it sounds like the system monitor engine is doing something wrong. Alex Merry wrote: System monitor maintains its list of sources (ie: things that can be monitored) by maintaining a bunch of DataContainers. Ordinarily, these just have some meta-information about the thing being monitored, such as a human-readable name and the units. Only when a source is connected to are the values updated. This is not an unusual approach to take, as far as I'm aware - that's kind of the point of DataContainer, right? It's just that systemmonitor doesn't bother subclassing DataContainer, but instead uses it directly. Note that the way KSysGuard works means that if it fetches the list of things that can be monitored, it has to fetch the meta-info as well, so it may as well store it. The issue is simply that it has to fetch this asynchonously, and visualizations may request a source before it has done so. This is not an unusual approach to take no, it is quite unusual. a DataContainer, once added to an engine due to a sourceRequestEvents not only represents that source but the source has the lifespan of the request. instead, what it ought to do is add all the DataContainers using DataEngine::addSource(DataContainer *) and then update them in updateRequestEvent. this behaviour can be triggered with setMinimumPollingInterval(0). this is documented in the apidox and is the way to get the desired behavior. so, yes, SystemMonitor is at fault here :) - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5312/#review7530 --- On 2010-09-10 23:30:32, Alex Merry wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5312/ --- (Updated 2010-09-10 23:30:32) Review request for Plasma. Summary --- Allow DataEngine implementations to prevent DataEngine from automatically deleting sources create in sourceRequestEvent. Rationale: engines that fetch a list of sources asynchronously at creation time (such as systemmonitor) may reasonably want to create dummy data sources when they are requested before the list has been fetched. However, they probably don't want these to be removed again when disconnected from. Diffs - /trunk/KDE/kdelibs/plasma/dataengine.cpp 1173953 /trunk/KDE/kdelibs/plasma/private/dataengine_p.h 1173953 /trunk/KDE/kdelibs/plasma/dataengine.h 1173953 Diff: http://svn.reviewboard.kde.org/r/5312/diff Testing --- It makes the systemmonitor engine work properly. Specifically, it solves the bug in bubblemon where if you had the bubblemon displaying, say, CPU Total Load when plasma started and then changed it to, say, CPU Idle Load, the CPU Total Load source would be removed and there would be an empty place where it should be in the list in the config dialog. Thanks, Alex ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow hiding of items in the Kate Sessions Plasmoid
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4264/#review7430 --- Ship it! i've comment on some minor improvements to the code and UI that should be made, imho, but the patch looks generally ready. if you can tidy up these last few items, feel free to commit this improvement! thanks for the patch... :) /trunk/KDE/kdesdk/kate/plasma/session/katesessionConfig.ui http://svn.reviewboard.kde.org/r/4264/#comment7633 this label is probably unnecessary: checking items is well understood to mean select, and with the title just being Sessions to show what that selection means will be clear. /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.h http://svn.reviewboard.kde.org/r/4264/#comment7636 following naming conventions, this should be: QStringList hideList() const; /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.cpp http://svn.reviewboard.kde.org/r/4264/#comment7634 i wouldn't bother with m_hideList in the KateSessionApplet class. the only time it is used, it is read from the configuration file first! instead, here i would just put: const QStringList hideList = config().readEntry(hideList, QStringList()); /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.cpp http://svn.reviewboard.kde.org/r/4264/#comment7632 perhaps just Sessions to show /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.cpp http://svn.reviewboard.kde.org/r/4264/#comment7635 without m_hideList this just becomes: config().writeEntry(hideList, m_config-hideList()); - Aaron On 2010-09-06 06:31:35, Yuen Hoe Lim wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4264/ --- (Updated 2010-09-06 06:31:35) Review request for Kate and Plasma. Summary --- Allows the user to specify which items to show or hide in the Kate Sessions Plasmoid using a configuration interface. Two possible use-cases for this: 1) Allow users who just want quick access to their projects to remove the default three items (Start Kate, New Session etc), which can take up a lot of space since the applet is usually quite small. 2) Allow multiple applets displaying different lists, for example having applets in different activities each displaying only sessions related to that activity. The Kate Sessions Plasmoid currently does not have a configuration interface so I thought this is probably no harm :) I have attached a screenshot of the configuration UI. I'm somewhat inexperienced with QtDesigner so feedback on how I could improve the patch code or the config UI are most welcome :) Diffs - /trunk/KDE/kdesdk/kate/plasma/session/CMakeLists.txt 1171815 /trunk/KDE/kdesdk/kate/plasma/session/katesessionConfig.ui PRE-CREATION /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.h 1171815 /trunk/KDE/kdesdk/kate/plasma/session/katesessionapplet.cpp 1171815 Diff: http://svn.reviewboard.kde.org/r/4264/diff Testing --- Tested on trunk on my computer. Works as far as I can tell. Screenshots --- Config UI Screenshot http://svn.reviewboard.kde.org/r/4264/s/428/ Updated config UI screenshot http://svn.reviewboard.kde.org/r/4264/s/497/ Thanks, Yuen Hoe ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: New Applet handle system
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5155/ --- (Updated 2010-09-04 17:44:10.408413) Review request for Plasma. Changes --- we get notifications via the plasma group; no point in getting everything twice. Summary --- This is a rewamp of the Applet handle system. Through its modular architecture it easily allows modifications and reuse of code. It features a base Handle class, AbstractHandle, and a base class for the control elements, ControlElement. I developed an handle based on the actual AppletHandle, DesktopHandle, and the control elements for the usual operations. Diffs - trunk/KDE/kdelibs/plasma/CMakeLists.txt 1171409 trunk/KDE/kdelibs/plasma/applet.h 1171409 trunk/KDE/kdelibs/plasma/applet.cpp 1171409 trunk/KDE/kdelibs/plasma/containment.h 1171409 trunk/KDE/kdelibs/plasma/containment.cpp 1171409 trunk/KDE/kdelibs/plasma/extenders/extender.cpp 1171409 trunk/KDE/kdelibs/plasma/extenders/extenderitem.cpp 1171409 trunk/KDE/kdelibs/plasma/handles/abstracthandle.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/abstracthandle.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/abstractcontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/abstractcontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/abstractcontrol_p.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/configurecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/configurecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/maximizecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/maximizecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/movecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/movecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/removecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/removecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/resizecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/resizecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/rotatecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/controlelements/rotatecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/handles/desktophandle.h PRE-CREATION trunk/KDE/kdelibs/plasma/handles/desktophandle.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/private/applet_p.h 1171409 trunk/KDE/kdelibs/plasma/private/applethandle.cpp 1171409 trunk/KDE/kdelibs/plasma/private/applethandle_p.h 1171409 trunk/KDE/kdelibs/plasma/private/containment_p.h 1171409 Diff: http://svn.reviewboard.kde.org/r/5155/diff Testing --- It isn't finished. It's missing the touch events management (which, however, it's hard for me to do, 'cause i don't have any touch screen device) and a better drag and drop system between containments. I'd like, however, to know what you think about what i've done, especially about the architecture. What's here works, though. Thanks, Giulio ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: New Applet handle system
On 2010-09-03 15:59:17, Aaron Seigo wrote: the patch does not apply cleanly to trunk; it needs to be updated / regenerated. instead of putting all of these classes into private/, i think it may make more sense to start a new dir in kdeliba/plasma/ called e.g. handles/, much as we did for animations. i've also included some comments on ControlElement as a starting point. Giulio Camuffo wrote: the patch was done against the last revision. i don't know what to do about it. your editor removed spaces at the ends of three lines (one in applet.cpp, one in containment.cpp and one in extender.cpp) but the patch does not show this change in it. not sure exactly what bit is messing up here (i'm guessing the svn diff), but i'll commit with those spaces gone and you should be able to re-generate the diff successfully at that point. try r1171395 of kdelibs/plasma. On 2010-09-03 15:59:17, Aaron Seigo wrote: trunk/KDE/kdelibs/plasma/private/controlelement_p.h, line 38 http://svn.reviewboard.kde.org/r/5155/diff/2/?file=34958#file34958line38 a pointer to the handle shouldn't be necessary. Giulio Camuffo wrote: but the handle provides a pointer to the containment and some things like position(). The ResizeControl must ask the handle to know which corner to keep static. the handle should not be required for such implementation specifics: * the containment comes from whatever creates the element. e.g. the Applet. * details such as which corner to anchor on should probably be sent to the Control by the Handle during mouse events i think one of the things that makes this difficult right now is that the handle is simply forwarding on mouse events. it would be more useful to know, e.g., if the item is simply triggerable (meaning all it needs to know is that the mouse was pressed and then subsequently released in its bounding rect, in the case of the current handles) or if the item needs to do mouse tracking. in both cases, a triggered call would be made (in the mouse tracking case, on mouse press; in the no-tracking case, on successfully mouse release) and then only mouse movement would need to be forwarded in the mouse tracking case (e.g. resize control). in the mouse tracking case, additional information could be sent, such as the anchor corner, using a subclass of QGraphicsMouseEvent. information about the cause of the triggering (e.g. which mouse button(s)) could be passed to the trigger(..) method. in the end, the ControlElement and the Handle should be very loosely coupled: it should be possible to change the Handle to a completely different class without touching ControlElement. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5155/#review7383 --- On 2010-09-01 16:22:54, Giulio Camuffo wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5155/ --- (Updated 2010-09-01 16:22:54) Review request for Plasma, Aaron Seigo and Marco Martin. Summary --- This is a rewamp of the Applet handle system. Through its modular architecture it easily allows modifications and reuse of code. It features a base Handle class, AbstractHandle, and a base class for the control elements, ControlElement. I developed an handle based on the actual AppletHandle, DesktopHandle, and the control elements for the usual operations. Diffs - trunk/KDE/kdelibs/plasma/CMakeLists.txt 1170608 trunk/KDE/kdelibs/plasma/applet.h 1170608 trunk/KDE/kdelibs/plasma/applet.cpp 1170608 trunk/KDE/kdelibs/plasma/containment.h 1170608 trunk/KDE/kdelibs/plasma/containment.cpp 1170608 trunk/KDE/kdelibs/plasma/extenders/extender.cpp 1170608 trunk/KDE/kdelibs/plasma/extenders/extenderitem.cpp 1170608 trunk/KDE/kdelibs/plasma/private/abstracthandle.h PRE-CREATION trunk/KDE/kdelibs/plasma/private/abstracthandle.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/private/applet_p.h 1170608 trunk/KDE/kdelibs/plasma/private/applethandle.cpp 1170608 trunk/KDE/kdelibs/plasma/private/applethandle_p.h 1170608 trunk/KDE/kdelibs/plasma/private/configurecontrol.h PRE-CREATION trunk/KDE/kdelibs/plasma/private/configurecontrol.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/private/containment_p.h 1170608 trunk/KDE/kdelibs/plasma/private/controlelement.h PRE-CREATION trunk/KDE/kdelibs/plasma/private/controlelement.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/private/controlelement_p.h PRE-CREATION trunk/KDE/kdelibs/plasma/private/desktophandle.h PRE-CREATION trunk/KDE/kdelibs/plasma/private/desktophandle.cpp PRE-CREATION trunk/KDE/kdelibs/plasma/private
Re: Review Request: Start Present windows by ctrl+left click on a pager indicator
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/5035/#review7253
---
i really dislike such 'magic' features for the reason of trustability. a person
will learn patterns of behaviour in the software and expect it to remain within
those patterns. whenever the software deviates, the user learns to expect
surprises and trust it less. this results in the user being more careful and
paying more attention to what they click on, etc. rather than just forgetting
about the environment and using it with as little conscious processing as
possible. like an experience driver behind the wheel of a car. (which is why
the idea of cars that accelerate on their own was so disruptive of an idea even
outside of the idea of possible harm and injury.)
unfortunately, we apparently have this feature in the tasks widget already. :/
in fact, it seems to have been part of another commit, probably by accident:
http://websvn.kde.org/?view=revisionrevision=997990
that commit message says, add a configuration ui for adding plasmoids into the
systray: only ones marked X-Plasma-NotificationArea=true in their desktop file
will show up in the list. it looks like the commit to taskgroup was an
accident.
in fact, that it only applies to groups and not to individual windows, that it
doesn't check for it being the root group, etc. makes me wonder about the
general quality of the feature. those things could be improved in the tasks
widget, of course, but personally i don't think this feature actually meets
what we're trying to achieve in terms of magic behaviours are bad.
it's been in there for a year, however, so i'm hesitant to yank it out at this
point. and if we leave it in the tasks widget, we should make it consistent
with the pager as well.
i'll leave it up to Marco since he was the one who made the original commit to
the tasks widget.
trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp
http://reviewboard.kde.org/r/5035/#comment7364
view() isn't guaranteed; to be on the (probably overly) safe side, it
should be:
QGraphicsView *v = view();
if (v) {
Plasma::WindowEffect::...
}
same below
- Aaron
On 2010-08-28 07:49:57, Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/5035/
---
(Updated 2010-08-28 07:49:57)
Review request for Plasma, Aaron Seigo and Marco Martin.
Summary
---
This patch adds support to pager to trigger the present windows effect for
the desktop the user clicks on when holding ctrl. This is similar to what we
have when clicking on a tasks group when ctrl is hold.
I'm not sure if this is a too hidden feature, but I consider it as could be
useful and consistent with the tasks applet.
Diffs
-
trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1163977
Diff: http://reviewboard.kde.org/r/5035/diff
Testing
---
Thanks,
Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Start Present windows by ctrl+left click on a pager indicator
On 2010-08-28 14:56:56, Todd wrote: Would it be possible to also do it with middle-click? This is unused for that widget as far as I can tell, and would avoid needing to use the keyboard entirely. that would be even more easy to trigger accidentally and raise the possibility of this being found not on purpose. imagine what someone who accidentally middle clicks and triggers this behaviour is going to think? most people won't connect I just middle clicked on the representation of a window with the present windows effect being triggered, and if they do it will likely come across as slightly odd. middle clicking doesn't have a bring forward and show semantic anywhere else afaik, ergo the unexpectedness. so, no. :) (tangentially, the same reason i don't like this feature is the same reason i veto'd the concept of scroll wheel over the battery plasmoid adjusting backlight brightness.) - Aaron --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5035/#review7250 --- On 2010-08-28 07:49:57, Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5035/ --- (Updated 2010-08-28 07:49:57) Review request for Plasma, Aaron Seigo and Marco Martin. Summary --- This patch adds support to pager to trigger the present windows effect for the desktop the user clicks on when holding ctrl. This is similar to what we have when clicking on a tasks group when ctrl is hold. I'm not sure if this is a too hidden feature, but I consider it as could be useful and consistent with the tasks applet. Diffs - trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1163977 Diff: http://reviewboard.kde.org/r/5035/diff Testing --- Thanks, Martin ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Launcher support for libtaskmanager
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4585/#review7215 --- Ship it! looks good :) let's get this into svn and then see about adding support to the tasks widget for launchers. - Aaron On 2010-08-25 18:12:01, Anton Kreuzkamp wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4585/ --- (Updated 2010-08-25 18:12:01) Review request for Plasma. Summary --- Adds support for Windows 7 like launchers in libtaskmanager. (I'm on holliday from 12th July until 1st August so I will not be able to reply during this time.) Diffs - /trunk/KDE/kdebase/workspace/libs/taskmanager/CMakeLists.txt 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h PRE-CREATION /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/kustodiangroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/programgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.cpp 1148442 Diff: http://reviewboard.kde.org/r/4585/diff Testing --- Tested with a small test-applett and everything works. Edit: In GroupManagerPrivate::removeTask(TaskPtr task) the method task-classClass()(l.326) returns an empty string (in GroupManagerPrivate::addTask(TaskPtr task) the method item-task()-classClass()(l.289) works just fine), so Launchers don't get showed again after quitting the application. Thanks, Anton ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add Show Icon only option to the tasks applet
On 2010-08-23 09:00:48, Marco Martin wrote: this very patch appeared here for several times already. and as usual, the question is: what real value gives over auto hiding the text when there is not enough space? Markus Slopianka wrote: If this patch works with the other one that implements launcher support, a Mac OS X-like Dock (AFAIK it's similar in Win7) can be implemented without the need to get 3rd party widgets. With a Dock-like setup I wouldn't want text other than tooltips. Beat Wolf wrote: i would actually agree on adding it from the feedback i get when i show kde to people used to windows. it's one of the first things they ask for. Aaron Seigo wrote: the number of such features that have appeared over the years is immense, and always people ask for those features ... as long as they are new in Windows. there is no point in chasing taillights just to chase taillights. if the idea is a good one, let's do it; if it isn't, let's not. Beat Wolf wrote: sure. i was just mentioning it because i for myself think its a good idea and other people want it, so from my point of view if the patch has a good quality, and it's actually using a feature that is there anyway, why not have it. But i see the other point of views too and all have their merit i think. Todd wrote: I think that the question is not so much a question of why we shouldn't have titles in the task manager, the question is why we need them. With grouping and the ability in 4.5 to change between grouped windows by clicking on their thumbnail, at least for me titles are just wasted space. I understand that for people without compositing they are needed, and some people may still prefer them, but personally they don't contribute anything, and they are huge relative to just the icons. I agree that the the current configuration interface for the task manager is getting a bit crowded. However, there is a way to add the feature without increasing the complexity of the dialog. Currently there are three grouping options: Do not group, Manually, and By Program Name. There is also an checkox Only when taskbar is full. The problem is that this option is only meaningful in By Program Name mode, and in fact the checkbox is disabled when the other two modes. So I would suggest getting rid of the checkbox and adding a fourth option in the dropdown When taskbar is full, or something along those lines. Also, since plasma supports multiple categories in the configuration dialog, it may be worthwhile splitting the current options into categories. Marco Martin wrote: With grouping and the ability in 4.5 to change between grouped windows by clicking on their thumbnail, at least for me titles are just wasted space. well, i think in this case is really fake wasted space because i would agree if the text would let to have less icons in the taskbar. but since when there is not enough room the text gets disabled automatically, this is a no issue. without that i really don't see use cases except making it look like windows Todd wrote: It isn't fake wasted space, there is still a bunch of text on there that fills up the area while contributing nothing to me. I don't think it looks good. This has nothing whatsoever to do with making it look like windows, I couldn't care less what windows does. It has to do with not making it look cluttered and inelegant. The text also contributes to the resizing of the panel, making the panel much larger than it has to be. I could, of course, force the panel to be small, but that works against me when I do have a lot of open windows, since it drastically limits the number of windows I can work with easily. Further, when the text is removed the tasks still expand to fill the available space, which looks really bad in my opinion. It makes sense when you want to show the text is hidden, but not when you don't want to deal with the text at all. contributing nothing to me * a larger target (Fitt's Law) * disambiguation from other similar items that may not matter to you, and i fully grant that. it matters to others, and we (the maintainers of this item) do not see enough value in option to turn the items into icons-only to include it in the tasks plasmoid directly. i have, however, already described a way that you (and whomever else) can accomplish your goals. we aren't exclusive, plasma has been designed to allow differences of opinion, differences of goals. others have taken advantage of this with stasks, fancytasks, etc. you can do the same without having to convince anyone or ask anyone's permission. so instead of continuing this conversation which is going to lead nowhere other than to find out what we already know (namely: we disagree on this matter), let's get back to hacking. as soon as your
Re: Review Request: Launcher support for libtaskmanager
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4585/#review7175 --- /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp http://reviewboard.kde.org/r/4585/#comment7316 if the item is guaranteed to be a LauncherItem, just use a static_cast (faster than a qobject_cast, and doesn't imply that it might be something else) /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h http://reviewboard.kde.org/r/4585/#comment7319 following the naming conventions: GroupItemType, LauncherItemType, TaskItemType /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp http://reviewboard.kde.org/r/4585/#comment7320 const QString /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp http://reviewboard.kde.org/r/4585/#comment7321 probably a bit faster to do: taskClass.compare(launcher-name(), Qt::CaseInsensitive) == 0 since that prevents a copy of the string being made. it would also make the above toLower() unnecessary. /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp http://reviewboard.kde.org/r/4585/#comment7317 the result of qobject_cast isn't checked and the item is guaranteed to be a launcher, so a static_cast is enough (and faster) here /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp http://reviewboard.kde.org/r/4585/#comment7318 the result of qobject_cast isn't checked and the item is guaranteed to be a launcher, so a static_cast is enough (and faster) here - Aaron On 2010-08-22 10:39:29, Anton Kreuzkamp wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4585/ --- (Updated 2010-08-22 10:39:29) Review request for Plasma. Summary --- Adds support for Windows 7 like launchers in libtaskmanager. (I'm on holliday from 12th July until 1st August so I will not be able to reply during this time.) Diffs - /trunk/KDE/kdebase/workspace/libs/taskmanager/CMakeLists.txt 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h PRE-CREATION /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/kustodiangroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/programgroupingstrategy.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1148442 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1166310 /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.cpp 1148442 Diff: http://reviewboard.kde.org/r/4585/diff Testing --- Tested with a small test-applett and everything works. Thanks, Anton ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add Show Icon only option to the tasks applet
On 2010-08-23 09:00:48, Marco Martin wrote: this very patch appeared here for several times already. and as usual, the question is: what real value gives over auto hiding the text when there is not enough space? Markus Slopianka wrote: If this patch works with the other one that implements launcher support, a Mac OS X-like Dock (AFAIK it's similar in Win7) can be implemented without the need to get 3rd party widgets. With a Dock-like setup I wouldn't want text other than tooltips. Beat Wolf wrote: i would actually agree on adding it from the feedback i get when i show kde to people used to windows. it's one of the first things they ask for. the number of such features that have appeared over the years is immense, and always people ask for those features ... as long as they are new in Windows. there is no point in chasing taillights just to chase taillights. if the idea is a good one, let's do it; if it isn't, let's not. - Aaron --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5078/#review7162 --- On 2010-08-22 13:52:33, Björn Ruberg wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5078/ --- (Updated 2010-08-22 13:52:33) Review request for Plasma. Summary --- This patch adds the option to put the taskbar in an icon-only mode - similar as in Windows 7 . This is an much requested feature in bugzilla. It is fairly simple and just using features already existing in the code, adding an m_showIconOnly member to the layout and the abstractitem plus the adaption of the config ui. This addresses bug 159480. https://bugs.kde.org/show_bug.cgi?id=159480 Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasksConfig.ui 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasks.cpp 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.h 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskitemlayout.cpp 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/abstracttaskitem.h 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/abstracttaskitem.cpp 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskgroupitem.h 1166313 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/taskgroupitem.cpp 1166313 Diff: http://reviewboard.kde.org/r/5078/diff Testing --- Moved panel around and made sure it works. Looks actually pretty good this icon-only mode! Thanks, Björn ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Enabling ShowDashboard widget to show Dashboard while anything is dragged and hovered over it.
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5063/#review7140 --- Ship it! one small comment about the timeout #, but this patch can go in.. cheers and thanks for the patch :) trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.cpp http://reviewboard.kde.org/r/5063/#comment7277 testing with this, i found 750ms more natural feeling: it didn't trigger accidentally as easily and it didn't feel like it took very long to trigger it either. - Aaron On 2010-08-19 16:09:42, Sinny Kumari wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5063/ --- (Updated 2010-08-19 16:09:42) Review request for Plasma. Summary --- Added feature to drag anything and adding it directly to the DashBoard. It can be done by dragging anything like file/widget and hovering it over ShowDashBoard widget . Diffs - trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.h 1164952 trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.cpp 1164952 Diff: http://reviewboard.kde.org/r/5063/diff Testing --- Tested it on trunk.It's working absolutely fine and i really enjoyed it while doing :) Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Enabling ShowDashboard widget to show Dashboard while anything is dragged and hovered over it.
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5063/#review7134 --- trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.cpp http://reviewboard.kde.org/r/5063/#comment7274 my only concern with this (and now that i think about it, the other patch as well) is that this will trigger _instantly_ on drag enter which means you can no longer drag across this widget without triggering it. that means that this will become a no dragging zone that the user will have to avoid while dragging things elsewhere. this is easily fixed though: have a QTimer connected to toggleShowDashboard() that is started in dragEnterEvent with a small timeout (e.g. 300ms) and which is stopped in dragLeaveEvent. that way, if the mouse stays in the widget for at least that amount of time with a drag, it triggers. but if it enters and leaves quickly (just being passed through) then nothing happens. - Aaron On 2010-08-18 15:15:58, Sinny Kumari wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5063/ --- (Updated 2010-08-18 15:15:58) Review request for Plasma. Summary --- Added feature to drag anything and adding it directly to the DashBoard. It can be done by dragging anything like file/widget and hovering it over ShowDashBoard widget . Diffs - trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.h 1164952 trunk/KDE/kdeplasma-addons/applets/showdashboard/showdashboard.cpp 1164952 Diff: http://reviewboard.kde.org/r/5063/diff Testing --- Tested it on trunk.It's working absolutely fine and i really enjoyed it while doing :) Thanks, Sinny ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
