> On Juli 12, 2014, 2:11 nachm., 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!!!
> 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?
On Juli 12, 2014, 2:11 nachm., Marko Käning wrote:
> > Other than that it looks ok. Please update the patch though.
>
> Ian Wadham wrote:
> I have tested the patch on Apple OS X in my kdesrc-build environment for
> KDE 4.13 branch. Before I did so, I removed the comment from line 316 and
> also the pair of braces from the line above it. Here are my results, using 3
> apps for which I know the source code (I am the curent maintainer).
>
> MAC - Palapeli, Create Puzzle - m_imageSelector(new KUrlRequester) [1]
> KDE - Palapeli, Import Puzzle(s) -
> KFileDialog::getOpenFileNames(KUrl("kfiledialog:///palapeli-import"), filter);
> MAC - Palapeli, Export Puzzle(s) -
> KFileDialog::getSaveFileName(KUrl(startLoc), filter) [2]
>
> MAC - Kubrick, Load Puzzle - KFileDialog::getOpenFileName (KUrl(),
> "*.kbk", myParent, i18n("Load Puzzle"))
> MAC - Kubrick, Save Puzzle - KFileDialog::getSaveFileName (KUrl(),
> "*.kbk", myParent, i18n("Save Puzzle"))
>
> KDE - KSudoku, Load -
> KFileDialog::getOpenUrl(KUrl("kfiledialog:///ksudoku"), QString(), this,
> i18n("Open Location"))
> KDE - KSudoku, Save -
> KFileDialog::getSaveUrl(KUrl("kfiledialog:///ksudoku"))
>
> [1] KUrlRequester is a mini-dialog with a text-edit box and a button. The
> button brings up a dynamic KFileDialog window in native Mac style.
>
> [2] QString startLoc =
> QString::fromLatin1("kfiledialog:///palapeli-export/%1.puzzle").arg(cmp->metadata.name);
> but startLoc does not appear in the dialog's location box as a default name.
>
> In each case, you have the dialog style that appeared (MAC or KDE),
> followed by the program and action, the invocation of KFileDialog used and
> maybe a note.
>
> Without the patch, using my MacPorts installation as at KDE 4.12.5, *all*
> the above cases gave the KDE dialog.
>
> When I inserted Native=true in
> ~/Library/Preferences/KDE/share/config/kdeglobals (which is where app
> preferences are kept in Apple OS X and MacPorts), still without any patch,
> there were some changes in "Palapeli, Create", "Kubrick, Load" and "Kubrick,
> Save", but all the others stayed at KDE style. "Palapeli, Create" with just
> Native=true (no patch) displayed a dialog that was empty except for buttons
> Cancel and OK. Kubrick displayed a MAC dialog in both cases.
>
> This post by Boudewijn Rempt is also relevant to this somewhat confusing
> situation and shows some more test results on KFileDialog on various
> platforms --- http://mail.kde.org/pipermail/kde-mac/2014-July/001211.html ---
> that thread is where René originally raised this topic.
The "problem" with the code is (likely) that "kfiledialog://" is not considered
as "local" protocol, so actually the only thing that confuses me is that the
patch turns "Palapeli, Export Puzzle" into a native dialog while the "Native"
key does not.
I assume this could be fixed (for KF5) by resolving "kfiledialog:///foobar"
first and then passing the result to the native dialog (if a local protocol) -
as is done for ::getSaveFileName, but not ::getSaveUrl or others.
The code in question is really messy in this context ;-)
Defaulting "Native" as true on OSX (and windows?) should then be sufficient - I
assume the conflicting behavior on ::getSaveFileName is due to the static flag,
polluted by ::getOpenFileNames().
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119243/#review62184
-----------------------------------------------------------
On Juli 14, 2014, 6:15 nachm., Marko Käning wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119243/
> -----------------------------------------------------------
>
> (Updated Juli 14, 2014, 6:15 nachm.)
>
>
> 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
>
>
