Review Request: Fix Open Picture.... menu behavior in Picture Frame applet

[Shantanu Tushar Jha]

> On 2009-07-02 13:09:12, Chani wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp, line 117
> > <http://reviewboard.kde.org/r/926/diff/1/?file=7775#file7775line117>
> >
> >     that if statement is getting awfully long. perhaps for the sake of readability you could split it up a bit with a bool or two? :)
> 
>  wrote:
>     I changed it to -
>     
>     bool invalidPicture = (m_currentUrl.path() == "Default") && (m_mySlideShow->currentUrl() == "Default");
>         if (hasAuthorization("LaunchApp") && ! (m_menuPresent || m_potd || invalidPicture)) {
>     
>     But noticed something weird, the function is somehow called twice so Open Picture menu is added twice !! (Even when I don't include the bool 'fix' above, obviously).
>     Both the times m_menuPresent is false, meaning that two calls are being made at the same time. I tried to figure out, but in vain.
>     Any idea?
> 
>  wrote:
>     I changed it to -
>     
>     bool invalidPicture = (m_currentUrl.path() == "Default") && (m_mySlideShow->currentUrl() == "Default");
>         if (hasAuthorization("LaunchApp") && ! (m_menuPresent || m_potd || invalidPicture)) {
>     
>     But noticed something weird, the function is somehow called twice so Open Picture menu is added twice !! (Even when I don't include the bool 'fix' above, obviously).
>     Both the times m_menuPresent is false, meaning that two calls are being made at the same time. I tried to figure out, but in vain.
>     Any idea?
> 
>  wrote:
>     sure, once from init() and once from updatePicture. there's a m_menuPresent = false in init() that's probably uneeded.

Done, removed the call from init(), because its anyway going to be called whenever the Picture updates. (I had commited after receiving first Ship It, will take care next time to wait for other people to comment).
Ok to commit this?


- Shantanu


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/926/#review1442
-----------------------------------------------------------


On 2009-07-02 09:15:41, Shantanu Tushar Jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/926/
> -----------------------------------------------------------
> 
> (Updated 2009-07-02 09:15:41)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Picture Frame didn't have Open Picture... option when a new Picture Frame is added and Slideshow is enabled. This patch fixes the problem.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp 990276 
> 
> Diff: http://reviewboard.kde.org/r/926/diff
> 
> 
> Testing
> -------
> 
> Tried on frame applet svn build with the patch. Works fine for me.
> 
> 
> Thanks,
> 
> Shantanu
> 
>