Re: Review Request: Workaround Taskbar ghost entries bug
On Friday, October 14, 2011 05:19:09 John Stanley wrote:
Hi,
I submitted a patch which hopefully fixes Bug
https://bugs.kde.org/show_bug.cgi?id=275469),
which may be related to this issue (see Comment #50). The patch is
short, so here it is:
--- kde-workspace-4.7.2.old/libs/taskmanager/taskitem.cpp
2011-05-20 16:32:08.0 -0400
+++ kde-workspace-4.7.2.new/libs/taskmanager/taskitem.cpp
2011-10-14 02:23:33.473139259 -0400
@@ -69,7 +69,6 @@ TaskItem::TaskItem(QObject *parent, Star
TaskItem::~TaskItem()
{
-emit destroyed(this);
there is a reason it is emitted in the dtor of TaskItem and not simply relying
on QObject to do it: the QObject signal happens too late for some parts of the
code which rely on it still being a TaskItem*. so this part is incorrect.
//kDebug();
/* if (parentGroup()){
parentGroup()-remove(this);
@@ -99,7 +98,7 @@ void TaskItem::setTaskPointer(TaskPtr ta
d-task = task.data();
connect(task.data(), SIGNAL(changed(::TaskManager::TaskChanges)),
this, SIGNAL(changed(::TaskManager::TaskChanges)));
-connect(task.data(), SIGNAL(destroyed()), this,
SLOT(deleteLater()));
+connect(task.data(), SIGNAL(destroyed(QObject*)), this,
SLOT(taskDestroyed()));
this is, however, correct, and was already commited in 4dedf3ce on July 31.
--
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43
KDE core developer sponsored by Qt Development Frameworks
signature.asc
Description: This is a digitally signed message part.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Re: Review Request: Workaround Taskbar ghost entries bug
Speaking of this bug, aseigo did you finish the debugging before you went on vacation ? if so, what was the conclusion ? (I'm refering to the potential bug in Qt not emitting destroyed signal). ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
On Tuesday, October 25, 2011 16:36:24 Alex Fiestas wrote: Speaking of this bug, aseigo did you finish the debugging before you went on vacation ? if so, what was the conclusion ? (I'm refering to the potential bug in Qt not emitting destroyed signal). before i left for vacation, i confirmed with Thiago that the event loop nestedness count is indeed screwing up. we don't know why, but the count is indeed wrong and that is why the deleteLater()s are never working. it seems to have something to do with x11EventFilter, as all the backtrackes go through there when the count is incorrect. so we have a couple of options: * try to figure out why x11EventFilter is wreaking havoc on the count * implement that unfortunate QTimer::singleShot(0) work-around along with a comment explaining why it is there i'm fine with either, mostly because i have time for neither at the moment :/ -- Aaron J. Seigo humru othro a kohnu se GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 KDE core developer sponsored by Qt Development Frameworks signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
Hi,
I submitted a patch which hopefully fixes Bug
https://bugs.kde.org/show_bug.cgi?id=275469),
which may be related to this issue (see Comment #50). The patch is
short, so here it is:
--- kde-workspace-4.7.2.old/libs/taskmanager/taskitem.cpp
2011-05-20 16:32:08.0 -0400
+++ kde-workspace-4.7.2.new/libs/taskmanager/taskitem.cpp
2011-10-14 02:23:33.473139259 -0400
@@ -69,7 +69,6 @@ TaskItem::TaskItem(QObject *parent, Star
TaskItem::~TaskItem()
{
-emit destroyed(this);
//kDebug();
/* if (parentGroup()){
parentGroup()-remove(this);
@@ -99,7 +98,7 @@ void TaskItem::setTaskPointer(TaskPtr ta
d-task = task.data();
connect(task.data(), SIGNAL(changed(::TaskManager::TaskChanges)),
this, SIGNAL(changed(::TaskManager::TaskChanges)));
-connect(task.data(), SIGNAL(destroyed()), this,
SLOT(deleteLater()));
+connect(task.data(), SIGNAL(destroyed(QObject*)), this,
SLOT(taskDestroyed()));
emit gotTaskPointer();
}
}
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102779/#review7107
---
the check makes sense; still wish we knew *where* these odd tasks were coming
from (my guess is that they are created and then deleted almost immediately
somewhere, but not before the tasks widget is told about them ..)
in any case, it looks like there is a potential problem with the new'ing of the
WindowTaskItem when the groupItem is invalid. if you could clear that up and
then commit, that'd be awesome. thanks for tracking this down! i know how much
time and effort you poured into this ... many will be thankful.
plasma/desktop/applets/tasks/taskgroupitem.cpp
http://git.reviewboard.kde.org/r/102779/#comment6235
looks like a possible memory leak (well, not quite; it's still parented
properly) due to the WindowTaskItem being new'd but not actually used.
it could be written sth like this instead:
} else {
TaskManager::TaskItem * taskItem =
static_castTaskManager::TaskItem*(groupableItem);
if (taskItem-startup() || taskItem-task()) {
WindowTaskItem *windowItem = new WindowTaskItem(this, m_applet);
windowItem-setTask(taskItem);
item = windowItem;
}
}
if (!item) {
return 0;
}
- Aaron J. Seigo
On Oct. 5, 2011, 9:26 a.m., Alex Fiestas wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102779/
---
(Updated Oct. 5, 2011, 9:26 a.m.)
Review request for Plasma and Aaron J. Seigo.
Description
---
Well, as some of you may know I have been tracking down the ghost entries
bug... this time I have NOT been able to fix the source of the problem but at
least after some GDB at BDS I think I have a workaround.
A Ghost entry happens when a TaskItem is not either a startup or a task, in
that case the code that paints the task doesn't know what to do but it
reserve the space anyway.
Currently the code path is like this:
1-TaskGruopItem::itemAdded is called
2-It calles TaskGruopItem::createAbstractItem
3-createAbstractItem takes a GroupableItem and cast it either to a GroupItem,
or LauncherItem or TaskItem depending on the ::itemTtype()
createAbstractItem is prepared to return 0 when an AbstractTaskItem can't be
created, and even further WindowTaskItem::setTask is checking if the taskItem
is not a startup or a task, the problem is that setTask returns void so we
can't really know if the setting was succeed or not. In that case, an
empty WindowTaskItem is returned, being it a ghost entry.
This workaround what does is check if the taskItem is valid, if it is not 0
is returned.
This is a workaround since, in theory a TaskItem should ALWAYS be either a
startup or a task so we should find the root of the problem instead of adding
yet another check. Is precisely this kind of checks that make all *tasks*
stack a mess.
Diffs
-
plasma/desktop/applets/tasks/taskgroupitem.cpp eec27c2
Diff: http://git.reviewboard.kde.org/r/102779/diff/diff
Testing
---
I have been using the patch for 3 days playing around with activities and
virtual desktops, so far so good.
Thanks,
Alex Fiestas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102779/#review7114 --- This review has been submitted with commit 681e1ef4b5f0f7e66dfa080badaaf9a7bac7 by Alex Fiestas to branch KDE/4.7. - Commit Hook On Oct. 5, 2011, 9:26 a.m., Alex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102779/ --- (Updated Oct. 5, 2011, 9:26 a.m.) Review request for Plasma and Aaron J. Seigo. Description --- Well, as some of you may know I have been tracking down the ghost entries bug... this time I have NOT been able to fix the source of the problem but at least after some GDB at BDS I think I have a workaround. A Ghost entry happens when a TaskItem is not either a startup or a task, in that case the code that paints the task doesn't know what to do but it reserve the space anyway. Currently the code path is like this: 1-TaskGruopItem::itemAdded is called 2-It calles TaskGruopItem::createAbstractItem 3-createAbstractItem takes a GroupableItem and cast it either to a GroupItem, or LauncherItem or TaskItem depending on the ::itemTtype() createAbstractItem is prepared to return 0 when an AbstractTaskItem can't be created, and even further WindowTaskItem::setTask is checking if the taskItem is not a startup or a task, the problem is that setTask returns void so we can't really know if the setting was succeed or not. In that case, an empty WindowTaskItem is returned, being it a ghost entry. This workaround what does is check if the taskItem is valid, if it is not 0 is returned. This is a workaround since, in theory a TaskItem should ALWAYS be either a startup or a task so we should find the root of the problem instead of adding yet another check. Is precisely this kind of checks that make all *tasks* stack a mess. Diffs - plasma/desktop/applets/tasks/taskgroupitem.cpp eec27c2 Diff: http://git.reviewboard.kde.org/r/102779/diff/diff Testing --- I have been using the patch for 3 days playing around with activities and virtual desktops, so far so good. Thanks, Alex Fiestas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102779/#review7115 --- This review has been submitted with commit 4fb69cc9bcda8974a220fe681116a866c03adb2f by Alex Fiestas to branch master. - Commit Hook On Oct. 5, 2011, 9:26 a.m., Alex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102779/ --- (Updated Oct. 5, 2011, 9:26 a.m.) Review request for Plasma and Aaron J. Seigo. Description --- Well, as some of you may know I have been tracking down the ghost entries bug... this time I have NOT been able to fix the source of the problem but at least after some GDB at BDS I think I have a workaround. A Ghost entry happens when a TaskItem is not either a startup or a task, in that case the code that paints the task doesn't know what to do but it reserve the space anyway. Currently the code path is like this: 1-TaskGruopItem::itemAdded is called 2-It calles TaskGruopItem::createAbstractItem 3-createAbstractItem takes a GroupableItem and cast it either to a GroupItem, or LauncherItem or TaskItem depending on the ::itemTtype() createAbstractItem is prepared to return 0 when an AbstractTaskItem can't be created, and even further WindowTaskItem::setTask is checking if the taskItem is not a startup or a task, the problem is that setTask returns void so we can't really know if the setting was succeed or not. In that case, an empty WindowTaskItem is returned, being it a ghost entry. This workaround what does is check if the taskItem is valid, if it is not 0 is returned. This is a workaround since, in theory a TaskItem should ALWAYS be either a startup or a task so we should find the root of the problem instead of adding yet another check. Is precisely this kind of checks that make all *tasks* stack a mess. Diffs - plasma/desktop/applets/tasks/taskgroupitem.cpp eec27c2 Diff: http://git.reviewboard.kde.org/r/102779/diff/diff Testing --- I have been using the patch for 3 days playing around with activities and virtual desktops, so far so good. Thanks, Alex Fiestas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
I don’t know if its related, but whilst working on IconTasks I noticed that items set to be deleted via deleteLater (in TaskGroupItem::itemRemoved and TaskManager::TaskItem) were not being deleted until after the app (plasmoidviewer / plasma-desktop) had terminated. To trace this I put debug when deleteLater was called, and then in the destructor. I have checked with the standard taskbar applet from KDE/4.7, and it has the same issue. I worked-around this by using a single-shot QTimer to call deleteLater() - but this seems a little hacky to me. Any ideas why the destructor is not being called? Craig. p.s. I also noticed a bug with AbstractTaskItem::setGeometry(). If the passed in geometry is the same as the current geometry, the layout animation and update-geometry timers need to be stopped. Otherwise a gap can appear when a group of items is closed all at once. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Workaround Taskbar ghost entries bug
On Wednesday, October 5, 2011 20:08:55 Craig Drummond wrote: I don’t know if its related, but whilst working on IconTasks I noticed that items set to be deleted via deleteLater (in TaskGroupItem::itemRemoved and TaskManager::TaskItem) were not being deleted until after the app (plasmoidviewer / plasma-desktop) had terminated. the only way deleteLater() will not delete is if the event loop is never entered. so i'm very suspicious about this claim; perhaps it looked like this is what was happening .. but i doubt that is what was happening, at least as described here ... however, if indeed, items are not getting deleted until app termination, that would _certainly_ explain the situations we're seeing. To trace this I put debug when deleteLater was called, and then in the destructor. I have checked with are you sure it was the same object (e.g. same address in memory?), and not just a task item with, e.g., the same name? I worked-around this by using a single-shot QTimer to call deleteLater() - but this seems a little hacky to me. can you share the patch that does this so i can see what is going on first- hand? thanks! :) p.s. I also noticed a bug with AbstractTaskItem::setGeometry(). If the passed in geometry is the same as the current geometry, the layout animation and update-geometry timers need to be stopped. Otherwise a gap can appear when a group of items is closed all at once. patches very much welcome for these kinds of issues :) -- Aaron J. Seigo humru othro a kohnu se GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 KDE core developer sponsored by Qt Development Frameworks signature.asc Description: This is a digitally signed message part. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
