D26749: WIP: Support NDK r20 and Qt 5.14

[Francis Herne]

flherne added a comment.


  I got my Kirigami app to load by patching kirigami.{cpp,h}:
  
  Altered `resolveFilePath()` to `return 
QStringLiteral("qrc:/android_rcc_bundle/qml/org/kde/kirigami.2/") + path;` and 
similarly for `resolveFileUrl()`.
  
  Added 
`QResource::registerResource(QStringLiteral("assets:/android_rcc_bundle.rcc"));`
 in `registerTypes()` [Qt is supposed to do this, but perhaps not early enough?]
  
  https://codereview.qt-project.org/c/qt/qtbase/+/270573/24 is the relevant Qt 
change.
  
  Icons are still broken.
  There are probably better ways to solve this.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26749

To: vkrause
Cc: flherne, apol, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D26749: WIP: Support NDK r20 and Qt 5.14

[Francis Herne]

flherne added a comment.


  Thanks for this.
  
  ADB log of the Kirigami problem, which results in a blank screen ['gredit' is 
the name of the app]:
  
libgredit_arm64-v8a.so: QQmlApplicationEngine failed to load component
libgredit_arm64-v8a.so: qrc:/main.qml:5:1: Type Kg.ApplicationWindow 
unavailable
libgredit_arm64-v8a.so: 
file:///data/app/org.qtproject.example.gredit-n9QHCzgETccPFueeH24YPQ==/lib/arm64/ApplicationWindow.qml:
 No such file or directory
  
  The `--Added-by-androiddeployqt--` dir in the APK, which previously contained 
these QML files, is no longer created.
  
  The file contents now seem to appear in an RCC file at 
`assets/android_rcc_bundle.rcc`, but clearly Kirigami isn't understanding this.
  
  Icons packaged by `kirigami_package_breeze_icons` no longer appear under 
`assets`.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26749

To: vkrause
Cc: flherne, apol, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D22069: Localize long number strings

[Francis Herne]

flherne added a comment.


   on IRC pointed out that this could affect telephone numbers.
  
  Also: numeric IDs [I bet at least something prints PIDs this way].
  
  I don't think it makes sense to try and enumerate specific cases -- this 
needs a flag to determine whether localization is done, and at least for KF5 
the default must be "no" to avoid breaking compatibility.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D22069

To: ngraham, #localization, #frameworks, broulik, aacid
Cc: flherne, dfaure, aacid, huftis, safaalfulaij, mikeroyal, aspotashev, ilic, 
kde-frameworks-devel, broulik, LeGast00n, GB_2, michaelh, ngraham, bruns

D22069: Localize long number strings

[Francis Herne]

flherne added a comment.


  I'm afraid this was a breaking API change -- it changes the behaviour, and 
not all numbers are intended to have separators added in this way.
  
  See https://bugs.kde.org/show_bug.cgi?id=413390
  
  A similar pattern occurs three different times just in KDevelop, and I can't 
even see how to work around this without breaking translations.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D22069

To: ngraham, #localization, #frameworks, broulik, aacid
Cc: flherne, dfaure, aacid, huftis, safaalfulaij, mikeroyal, aspotashev, ilic, 
kde-frameworks-devel, broulik, LeGast00n, GB_2, michaelh, ngraham, bruns

D17137: KTextEditor: File menu: Put Save, Print and Export in submenus

[Francis Herne]

flherne added a comment.


  Please no.
  
  `File -> Save` is one of the most universal menu entries, found in nearly 
every program that *has* a set of menus. Print is nearly as common.
  
  This breaks a user assumption that's been ingrained across virtually all GUI 
software for decades, and makes common operations more cumbersome (although I 
assume many users prefer the shortcuts), for the sake of a few lines in the 
menu.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17137

To: gregormi, #kate, #kdevelop
Cc: flherne, dhaumann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, cullmann, sars

[Differential] [Closed] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:a3355b22954b: KColorScheme: default to application scheme 
if set by KColorSchemeManager (authored by flherne).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4637?vs=11471=11508

REVISION DETAIL
  https://phabricator.kde.org/D4637

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks, graesslin
Cc: graesslin

[Differential] [Updated] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

flherne marked an inline comment as done.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Commented On] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

flherne added a comment.


  In https://phabricator.kde.org/D4637#87022, @graesslin wrote:
  
  > This is a sensible idea, though I wonder whether we should put it into 
something more concrete than a QProperty on the qApp.
  
  
  That's not new, the property's been set here 
 
since at least the kdelibs split.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Updated, 26 lines] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

flherne updated this revision to Diff 11439.
flherne added a comment.


  Tweak a comment.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4637?vs=11438=11439

REVISION DETAIL
  https://phabricator.kde.org/D4637

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Updated, 25 lines] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

flherne updated this revision to Diff 11438.
flherne added a comment.


  Moved defaultConfig() out of the header in case someone includes it.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4637?vs=11417=11438

REVISION DETAIL
  https://phabricator.kde.org/D4637

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Request, 25 lines] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

[Francis Herne]

flherne created this revision.
flherne added a reviewer: Frameworks.
flherne set the repository for this revision to R265 KConfigWidgets.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  [RFC]
  
  KColorSchemeManager::activateScheme()  sets a custom path for the 
application's color scheme, with
  
qApp->setProperty("KDE_COLOR_SCHEME_PATH", index.data(Qt::UserRole));
  
  Currently, the KColorScheme() and KStatefulBrush() constructors will ignore 
this and use only the system color scheme, unless an application-specifiic 
config is explicitly loaded and passed in by the caller.
  
  This is problematic, because all callers I've seen assume that the default is 
to match the //application// scheme (usually this is equivalent, because few 
applications use KColorSchemeManager).
  
  For example, when the application of a KTextEditor widget or KonsolePart has 
an opposite color scheme to the system, the Find bars are unreadable: 
https://bugs.kde.org/373764
  
  This patch makes KColorScheme() match the application scheme by default when 
this differs from the system scheme, which seems preferable to adding the same 
code in hundreds of callers.
  
  BUG: 373764

TEST PLAN
  It works...
  
  I haven't looked at proper tests yet.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks

[Differential] [Closed] D4421: Reverse meaning of :split, :vsplit to match vi and Kate actions.

[Francis Herne]

flherne closed this revision.
flherne added a comment.


  
https://cgit.kde.org/ktexteditor.git/commit/?id=b4006363eaf1d9d26f037bcdbc5f1a283b9d109e

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4421

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, michalhumpula, #ktexteditor, dhaumann
Cc: dhaumann, michalhumpula, cullmann, mwolff, kwrite-devel, #frameworks

[Differential] [Request, 6 lines] D4421: Reverse meaning of :split, :vsplit to match vi and Kate actions.

[Francis Herne]

flherne created this revision.
flherne added a reviewer: KTextEditor.
flherne set the repository for this revision to R39 KTextEditor.
Restricted Application added subscribers: Frameworks, kwrite-devel.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Reverse meaning of :split, :vsplit to match vi and Kate actions.
  
  The previous behaviour of these commands was swapped, compared either to vi 
or to "Split Vertical" in Kate.
  
  :new and :vnew were fixed in 
https://phabricator.kde.org/R39:0b00782a90ec1d68b0fe0f3140620aeb6de4b132, but 
this pair was missed.
  
  Given that every use I can find has this confusing reverse pattern, the API 
is probably wrong. Too late now.
  
  Pointed out by head7 in https://phabricator.kde.org/D4250.

TEST PLAN
  Compiles, matches vi now.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4421

AFFECTED FILES
  src/vimode/appcommands.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #ktexteditor
Cc: kwrite-devel, #frameworks

Re: Phabricator: All repositories registered - upcoming workflow changes

[Francis Herne]

On Thu, 2 Feb 2017 07:45:15 +1300
Ben Cooksley  wrote:
[snip]
> 
> I think that we need some cleanup on the old reviews (Albert Astal Cid
> started
> some time ago) and more important strongly tell new users (and old
> users) to use Phabricator. I don't think that anyone wants to lose
> the work, but if a review has not been touched in a few months maybe
> it's time to see it is still
> interesting.
> If we start doing this now (or yesterday), the flow of new patches in
> reviewboard should decrease quickly.
> 
> 
> Getting everyone to shift over was the whole point of this thread. No
> new reviews should be opened on ReviewBoard, and existing ones
> shouldnt take more than a few weeks to clear.
> 
> Anything older than that usually won't apply to the code anymore.
> 
[snip]
> 
> Regards,
> Ben

Not every project is Plasma!

The churn rate in most projects is very low - as an arbitrary
statistic, the mean age of lines in kdevplatform is about 6 years. [1]

Almost every review since the KF5 port at least is semantically
meaningful, even when a few lines have been changed that prevent the
patch from applying.



Completely aside from open reviews, the history of comments and
revisions to committed ones is indispensable.

I don't know if you've seen my IRC comments, but in many cases the
reviewboard comments are the only record of why things are done in a
particular way. There's much more information in them than the commit
history, especially when the commit message or code comment says
 "see ".

As a relative newcomer to the project, reading the reviews has saved me
literally hours compared to reverse-engineering the author's thought
process, and I'm one user out of dozens (hundreds?).


From another of your messages:
> There won't be any auto-forwarding - we'll be removing the subdomains
> completely.

Given the poor quality of KDE's mail indexing, this will make finding
comments for a given review somewhere between painful and impossible.

The logged update emails don't include the actual diffs *at all*, which
will make most comments meaningless in any case.

If you go ahead with this, we will lose years of accumulated knowledge
about the project, and it will cost many (many!) hours of developers'
time. I really did mean it when I wrote 'catastrophic'.

-Francis



[1]
`for file in $(git ls-files); do git blame --line-porcelain "$file"
| grep author-time; done | awk '{ total += ($2 /86400) } END { print
total/NR }'`
* 86400, to readable date, comes out as 22 May 2011.

Re: Phabricator: All repositories registered - upcoming workflow changes

[Francis Herne]

Sorry, forgot to reply-all and only sent to kdevelop-devel...

-

Hi,

First off, there's a lot of postponed, or at least possibly-useful,
work on ReviewBoard which would be lost. Some of this is from newish
contributors who might be discouraged - e.g. the author of
https://git.reviewboard.kde.org/r/129589/ mentioned on IRC the other
day that he's hoping to complete it at some point.

For already-committed work:

Even if the mail-archiving infrastructure was in a useful state, this
would be inconvenient - there are more than a *thousand* REVIEW: tags in
kdev* project commits, plus several comments with "see ".

Many mailing lists aren't logged at all, there's no internal
search with only patchy Google indexing, and 'browsing' the archive
means clicking through arbitrarily-grouped mails by date with minimal
threading. That's not merely inconvenient, it's going to cause a
catastrophic loss of information.

Please reconsider.

-Francis

On Wed, 1 Feb 2017 07:56:52 +1300
Ben Cooksley  wrote:

> On Tue, Jan 31, 2017 at 11:36 PM, René J.V. Bertin
>  wrote:  
> > On Sunday January 29 2017 08:32:21 Ben Cooksley wrote:
> >
> > Hi,
> 
> Hi Rene,
>   
> >
> > >From this point forward, communities should be moving away from
> >>Reviewboard to Phabricator for conducting code review. Sysadmin will
> >>be announcing a timeline for the shutdown of Reviewboard in the near
> >>future.
> >
> > I hope that shutdown doesn't mean complete disconnect; it would
> > probably be a loss of as-yet unknown importance if all code reviews
> > become unavailable.
> >
> > I'll miss ReviewBoard. Phabrithingy may be more powerful and
> > versatile, but RB had its advantages too which could be why it's
> > still being used (quite a lot, as far as I can see) and hasn't been
> > integrated with KDE's own IDE yet.
> 
> It will be a complete shutdown of Reviewboard - we'll be archiving it
> in the event for some reason it becomes necessary to access the data
> it stores.
> 
> In most cases mailing lists should have the history of reviews in
> their archives, so those will continue to be accessible through list
> archives in the long run.
> 
>   
> >
> > R.
> 
> Cheers,
> Ben