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