[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 119243: Better OSX integration: native file dialogs and unified title/toolbar
From: "Ian Wadham" <iandw.au () gmail ! com>
Date: 2014-07-22 0:52:42
Message-ID: 20140722005242.14616.86334 () probe ! kde ! org
[Download RAW message or body]
--===============4799865003086461792==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On July 21, 2014, 5:16 a.m., Ian Wadham wrote:
> > So where are we leaving this? Any conclusions? Any solutions or further patches?
> >
> > Should we report the inconsistent handling of Native file dialogs as a bug on \
> > bugs.kde.org (without a patch this time)?
> > If so, against what "product" should we report it (i.e. where a KDE developer \
> > will be listening)?
> > I agree with Thomas that "kfiledialog://" should be evaluated before deciding \
> > whether it is a local file.
> > I notice also, that in the "condition" that keeps occurring ((!startDir.isValid() \
> > || startDir.isLocalFile()), the isValid() method seems to be inherited from QUrl \
> > class and maybe isLocalFile() is too.
> > It seems odd that a possible condition for invoking QFileDialog methods should be \
> > that the QUrl is invalid. Perhaps there used to be an isValid() in KUrl with a \
> > different meaning.
> > It's all very confusing. Who can sort it out?
>
> Thomas Lübking wrote:
> dfaure (assined to the kio framework)
>
> kfiledialog.cpp would require a general cleanup, but it's in kde4support now, so \
> the predominant question is whether it makes any sense to spend time on this at \
> all.
The following change by dfaure (David Faure) says it all. KFileDialog is now \
deprecated in favour of QFileDialog. \
https://projects.kde.org/projects/frameworks/kdelibs4support/repository/diff/src/kio/k \
filedialog.h?rev=977151f9747bc3b7db6d68daafcee41e11516e31&rev_to=cb01fc274d4c0e907d27a1cd7fc80bbca9c1dbcd
When/if all uses of KFileDialog in KDE apps are replaced by uses of QFileDialog, all \
their file dialogs should become Native on all platforms.
Unfortunately, the above change has not been entered into \
https://community.kde.org/Frameworks/Porting_Notes so I do not know how KDE apps \
authors like me would normally become aware of it. I did check the Porting Notes \
carefully before entering into this discussion and I have just checked them again. If \
they had been up-to-date on this issue, that could have saved us all a lot of time on \
this review!
With the Randa sprint coming up soon (and hopefully lots of KDE apps porting being \
done), I would suggest that the Porting Notes get re-read in their entirety by the \
authors and updated soon, to catch this and any other missing bits, if that is not \
already happening.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119243/#review62755
-----------------------------------------------------------
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
>
>
--===============4799865003086461792==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119243/">https://git.reviewboard.kde.org/r/119243/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On July 21st, 2014, 5:16 a.m. UTC, <b>Ian \
Wadham</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">So where are we leaving this? Any conclusions? Any \
solutions or further patches?</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Should we report the \
inconsistent handling of Native file dialogs as a bug on bugs.kde.org (without a \
patch this time)?</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">If so, against what "product" should we \
report it (i.e. where a KDE developer will be listening)?</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
agree with Thomas that "kfiledialog://" should be evaluated before deciding whether \
it is a local file.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I notice also, that in the "condition" \
that keeps occurring ((!startDir.isValid() || startDir.isLocalFile()), the isValid() \
method seems to be inherited from QUrl class and maybe isLocalFile() is too.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">It seems odd that a possible condition for invoking QFileDialog methods \
should be that the QUrl is invalid. Perhaps there used to be an isValid() in KUrl \
with a different meaning.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">It's all very confusing. Who can sort \
it out?</p></pre> </blockquote>
<p>On July 21st, 2014, 11:52 a.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">dfaure (assined to the kio framework)</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">kfiledialog.cpp would require a general cleanup, but it's in kde4support \
now, so the predominant question is whether it makes any sense to spend time on this \
at all.</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The \
following change by dfaure (David Faure) says it all. KFileDialog is now deprecated \
in favour of QFileDialog. <br style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;" /> \
https://projects.kde.org/projects/frameworks/kdelibs4support/repository/diff/src/kio/k \
filedialog.h?rev=977151f9747bc3b7db6d68daafcee41e11516e31&rev_to=cb01fc274d4c0e907d27a1cd7fc80bbca9c1dbcd</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">When/if all uses of KFileDialog in KDE apps are \
replaced by uses of QFileDialog, all their file dialogs should become Native on all \
platforms.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Unfortunately, the above change has not been entered \
into https://community.kde.org/Frameworks/Porting_Notes so I do not know how KDE apps \
authors like me would normally become aware of it. I did check the Porting Notes \
carefully before entering into this discussion and I have just checked them again. If \
they had been up-to-date on this issue, that could have saved us all a lot of time on \
this review!</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">With the Randa sprint coming up soon (and hopefully \
lots of KDE apps porting being done), I would suggest that the Porting Notes get \
re-read in their entirety by the authors and updated soon, to catch this and any \
other missing bits, if that is not already happening.</p></pre> <br />
<p>- Ian</p>
<br />
<p>On July 14th, 2014, 6:15 p.m. UTC, Marko Käning wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Software on Mac OS X, kdelibs, Christoph Feck, Ian \
Wadham, and RJVB Bertin.</div> <div>By Marko Käning.</div>
<p style="color: grey;"><i>Updated July 14, 2014, 6:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=337356">337356</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This bundles both patches submitted by René J.V. \
Bertin in the associated issue</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">See issue for more info from René.</p> <hr \
style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px \
solid #ddd;line-height: inherit;" /> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">I myself haven't yet \
tested this. Will report back as soon as I got to it.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdeui/widgets/kmainwindow.cpp <span style="color: \
grey">(85beaccdb6f123d2996b8c2b5e38435265c63ff8)</span></li>
<li>kio/kfile/kfiledialog.h <span style="color: \
grey">(2b11796897ff095279e7c254a383a9e8e323ea0f)</span></li>
<li>kio/kfile/kfiledialog.cpp <span style="color: \
grey">(4005ba304a18b59572cc1aece3fcd122ce05fda9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119243/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============4799865003086461792==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic