search

psifidotos / workflow-project

This is an effort to create a KDE Plasmoid that integrates the main Activities, Virtual Desktops and Tasks Functionalities from Plasma Desktop in just one component.
http://workflow.opentoolsandspace.org/
GNU General Public License v2.0
11 stars 2 forks source link

Uninitialised variable win #37

Closed mdaffin closed 11 years ago

mdaffin commented 11 years ago

In file src/workflow.cpp line 451:

WId win;
activeWindowChanged(win);

win is never initialised so makes no scene to pass it to the function, uninitialised variables will just contain random junk which explains this comment

In file src/workflow.cpp line 251: //This slot is just to check the id for the dashboard //there are circumstances where this id changes very often, //this id is used for the window previews void WorkFlow::activeWindowChanged(WId w) # w is never used

This looks to me like you are working around the bug rather then fixing it.

I am not entirely sure what activeWindowChanged is meant to do. Is it just checking if it is in the dash board or not?

mdaffin commented 11 years ago

Looking at this in more detail it looks like the code is only used to check if the widget is in the dashboard or not and respond appropriately. The current implementation calls this method every time a window is changed, which seems wasteful since the widget is rarely going to move from the dashboard or to the dashboard let along every time a window is changed (even if the widget is not the active window).

It would be better to only call this method once at startup, or better to find a way to tell when the widget is moved to the dashboard. For now I suggest just setting it on startup as I don't see people moving it between the dashboard and back often enough to warrant a check every time the active window is changed.

mdaffin commented 11 years ago

I don't think this is the best final solution, as that would involve much larger changes, but it should work for now and is a step closer to cleaning up the code.

psifidotos commented 11 years ago

james the mentioned code is not doing just that.

The window previews in the dashboard need the window id in order to show. But the dashboard window id changes almost every time is shown. I believe that with your code if you are showing window previews in the dashboard there will be problems. Show / close the dashboard many times with window previews enabled. Arent there any problems?

mdaffin commented 11 years ago

Fair enough, there must still be a better way to handle this, probably passing the view as a pointer to the appropriate classes so the window id can be obtained directly rather then set all the time.

I will leave this for now and handle it during my homerun port instead as that is going to require some rewriting of the way things work.

Feel free to close this issue if you don't want to look at it any further.

psifidotos commented 11 years ago

Fixed with Q_UNUSED macro