Review Request 119243: Better OSX integration: native file dialogs and unified title/toolbar

[Ian Wadham]

> 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 =)

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!!!


- Ian


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


On July 12, 2014, 9:55 a.m., Marko Käning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119243/
> -----------------------------------------------------------
> 
> (Updated July 12, 2014, 9:55 a.m.)
> 
> 
> Review request for kdelibs, Christoph Feck and Ian Wadham.
> 
> 
> 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-core-devel/attachments/20140714/8001a73f/attachment.htm>