[KDE/Mac] Review Request 119243: Better OSX integration: native file dialogs and unified title/toolbar

Wed Jul 16 07:23:53 UTC 2014

> On July 12, 2014, 2:11 p.m., Aleix Pol Gonzalez wrote:
> > kio/kfile/kfiledialog.cpp, line 316
> > <https://git.reviewboard.kde.org/r/119243/diff/1/?file=289740#file289740line316>
> >
> >     I don't know why you did that, but it doesn't look good.
> 
> Marko Käning wrote:
>     Actually, when submitting this RR I was also stumbling over this commented out line and wondered why...
>     
>     I am afraid we've to wait for René to figure this out.
> 
> Ian Wadham wrote:
>     If we are going to use Review Board, can we get the kde-mac list added as a group? How do you do that?
>     
>     That would let René, Boudewijn and Nicolas listen in on this stuff, FWIW.
> 
> Ian Wadham wrote:
>     Actually I do not think Review Board is worth much.  It is extra work, hard to use and, in my experience, rarely results in anything much more than white-space adjustments - as you can see from the unanswered questions, issues and comments above, by Mark Gaiser, Marko and me.  Actually I answered Marko's issue, but I would far rather be doing so, more quickly and directly, on kde-mac or BKO, if none of the kdelibs maintainers have anything useful to add.  Nevertheless I will have one more try at reviewing this patch, now that I have tested it on Apple OS X with a few different file-open and save situations.
> 
> Thomas Lübking wrote:
>     You'd need to request addition of the group to groups (just a formality to add the entry, ask sysadmin æt kde döt org), otherwise you've to add rene (rjvbb) directly.
>     
>     The file dialog apparently requires the native fallback in case the start dir is not local.
>     The string UnifiedTitleAndToolBarOnMac is only used here: http://lxr.kde.org/search?_filestring=&_string=UnifiedTitleAndToolBarOnMac 
>     The key is "Native" in kdeglobals (~/.kde/share/config/kdeglobals), [KFileDialog Settings] group (just looked up the code on the static flag, see http://api.kde.org/4.x-api/kdelibs-apidocs/kio/html/kfiledialog_8cpp_source.html from line 206)
>     
>     A very specific problem about this review is that the submitter is not the author.
>     
>     The worth of reviewboard is (at least) to elimintate bad commits (see "commented d->w->setCustomWidget(QString(), customWidget);" - whatever the problem is, this solution not only adds cruft and lacks a comment but most certainly introduces a bug, since it basically kills the API feature)
>     
>     ... oh, and this review was opened on the FIFA cup finals weekend - eg. i still won't be near a usable workstation before tonight. And I think it's amazing™ that i can already use a keyboard again =)
> 
> Ian Wadham wrote:
>     Thanks, I have applied to add kde-mac to the groups.
>     
>     On your advice, I used the Native key (=true) to do some tests. It was not appearing in kdeglobals anywhere on Apple OS X because the default (false?) was always set and KConfig entries with default values are not shown. I wish KConfig would not do that.
>     
>     Actuallly the submitter to the submitter is not the author either. René acquired this patch from somewhere on KDE Forums IIRC and originally floated it on BKO. I appreciate your point about bad commits, but we (on KDE-Mac) are not complete newbies. Our usual practice would be to test such a change "downstream", individually at first and then, if it looks good, by releasing it in MacPorts. Then arises the question of how and where it should appear as a change in KDE portability code - in KDE 4, in KF 5, etc. My solution has been to set up https://projects.kde.org/projects/playground/base/osx-patches/repository/revisions/master/entry/README so that KDE and MacPorts people can decide for themselves when and in what version to incorporate such patches.
>     
>     As to the World Cup, well Marko submitted this review request and he is German too... :-) Never mind. Well done Germany!!! I am very happy to see you win!!!
> 
> Thomas Lübking wrote:
>     > I wish KConfig would not do that.
>     
>     I assume it's rather because the key has no GUI item and is (therefore) not considered in kdeglobals.kcfg - it's a "secret" to the library code.
>     
>     > we (on KDE-Mac) are not complete newbies
>     
>     That's actually not the point. By formalizing the process you more or less guarantee that the patch had at least a surface level cross-check. (That's not foolproof either, but better than nothing)
>     
>     "We just somehow do it, we're pros" works great for small scale groups, but not for things like KDE with hundreds of modules and thousands of occasional commiters.
>     
>     You do not need the policy to cover the individual idiot, but to handle the crowd (and the idiots inside ;-)
>     
>     Eg. I guess there're speed limits down under?
>     Ever thought why they are there? Because *you* would be too stupid to pick an appropriate speed?
>     Nahhh... It's to coordinate traffic. I guess there're no explicit limits in the outback, where there is about one car every once a week?
>     
>     > Then arises the question of how and where it should appear as a change in KDE portability code
>     
>     Bugfixes can go into kdelibs/workspace 4, new features into KF5 / "Plasma Next". Other modules/applications are not frozen.
>     
>     However looking randomly at some patches, they seem to cover actual bugs which just emerge on OSX.
>     Eg. KMyMoney should not crash on the raster graphicssystem, period. It's the only available in Qt5, so this will have to be fixed for KDE5 anyway. 
>     Other things like the libdispatch issue on kcrash could require a "proper" fix (isolate libdispatch FDs and exclude them from closing. Actually, unconditionally closing random FDs might be a more gereral issue and just worked by luck so far)
>     
>     -> Are there open bug reports for those issues?

Got a few minutes before vacation mode kicks in again :)

As Ian mentioned, I just created a usable patch from information in a forum post from 2011, which I then posted on the ML and MacPorts trac for others to play with. I don't know anything about KDE internals (so never meant for these patches to take on a "formal nature ;)) and there's no explanation in the forum post of the changes that do not amount to (as far as I can see) hard-coded choices to use a native dialog by default. So I really have no idea what the change involving `d->w->setCustomWidget(QString(), customWidget)` does.

- RJVB

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119243/#review62184
-----------------------------------------------------------

On July 14, 2014, 6:15 p.m., Marko Käning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119243/
> -----------------------------------------------------------
> 
> (Updated July 14, 2014, 6:15 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kdelibs, Christoph Feck, Ian Wadham, and RJVB Bertin.
> 
> 
> Bugs: 337356
>     http://bugs.kde.org/show_bug.cgi?id=337356
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> This bundles both patches submitted by René J.V. Bertin in the associated issue
> 
> 
> Diffs
> -----
> 
>   kdeui/widgets/kmainwindow.cpp 85beaccdb6f123d2996b8c2b5e38435265c63ff8 
>   kio/kfile/kfiledialog.h 2b11796897ff095279e7c254a383a9e8e323ea0f 
>   kio/kfile/kfiledialog.cpp 4005ba304a18b59572cc1aece3fcd122ce05fda9 
> 
> Diff: https://git.reviewboard.kde.org/r/119243/diff/
> 
> 
> Testing
> -------
> 
> See issue for more info from René.
> 
> ---
> 
> I myself haven't yet tested this. Will report back as soon as I got to it.
> 
> 
> Thanks,
> 
> Marko Käning
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140716/5dd738ee/attachment-0001.html>