[Kde-pim] Review Request: Have KMail behave more like other kdepim apps with regards to session management/systray

[Mathieu Seigneurin]

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


Thanks for looking!

Yes, I agree with you: it would be better if the "hidden" or not state was saved as part
of the KMainWindow properties (along with geometry & other state that's already saved).

Changing it at that level actually seems rather trivial (no harder than for kmail), but
it does have some consequences: the "restore(int num,bool show)" method of KMainWindow will show
the window by default (i.e. show=true in the .h, documented like that in the APO).
The kRestoreMainWindow(n) and RESTORE(n) template function/macro do not (currently) have a way
of passing down something different for "show".

So the current assumption & documentation says that a call to these the macro or function
template, or direct call to restore(n) _will_ show the window.

Two options that I seen:
1/ in KMainWindow, save an additional bool ("Hidden") along with state & geometry, but don't
  change any of the restore paths to keep the same assumptions/API
 => still needs changes in the derived classes to override the default on restore(n,bool), but
  at least the hidden property will be automatically saved/retreived and have a standard name.

2/ in addition to 1/, in KMainWindow::restore, do not honor the "bool show" parameter if the
  window was hidden during session save (which is what i did in this patch). This bothers me
  because without an API change there would no longer be a way of forcing the window to be
  shown on restore without coding it in the derived class. (We inverse the problem we have now of
  needing to force it to not show in the derived class.)

I guess we could do something like:
    kmainwindow.h:
        bool restore(int n);  // i.e. do away with the default show=true
        bool restore(int n, enum ShowMode);
Have the first method behave like restore currently does (maintaining consistency with the
api/docs), and the second one more configurable:
  ShowMode::AlwaysShow => always display on restore
  ShowMode::AlwaysHide => always hide on restore
  ShowMode::Default    => do as indicated by the saved "Hidden" property
And extend the RESTORE and kRestoreMainWindow things to allow this second mode of operation.

I'm a bit worried about binary compatibility in this case, and I'm not knowledgable enough
in C++ to see if that's a problem. Could try it out though - maybe this WE.

(Probably lots more possibilities, but that's what I've seen so far.)

All in all, the simple KMail fix isn't that bad (in my very unbiased opinion :-), even if it's
not perfect.

I'll look more into the KMainWin alternatives if you think it's worth it.

There are other things I'd like to do to KSystemTrayIcon (kdelibs one) to remove more code from
its users - i think it should be possible to remove most of the #ifdef ..X11 or #ifdef ..W32 in
the simple derived classes of KSystemTrayIcon, including KMail's one with a bit more flexibility
from the kdelibs implementation.

Anyway, many thanks for your feedback. This isn't a real important bug so no hurry at all.
It's just very anoying to me :-)

- Mathieu


On 2009-04-04 04:07:39, Mathieu Seigneurin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/504/
> -----------------------------------------------------------
> 
> (Updated 2009-04-04 04:07:39)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Hello,
> With this patch, KMail should start minimized to tray on session open if it was minimized to tray on session close.
> This makes it more consistent with e.g. aKregator (patch base entirely on aKregator) and might help close bug 75673.
> 
> Please be gentle - this is my first patch/review :-)
> 
> 
> This addresses bug 75673.
>     https://bugs.kde.org/show_bug.cgi?id=75673
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/kmmainwin.h 949028 
>   /trunk/KDE/kdepim/kmail/kmmainwin.cpp 949028 
> 
> Diff: http://reviewboard.kde.org/r/504/diff
> 
> 
> Testing
> -------
> 
> Works for me on KDE 4.2.2 (gentoo). No idea if/how a testcase can be written.
> 
> 
> Thanks,
> 
> Mathieu
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/