D29254: [RenameDialog] Add an arrow indicating direction from src to dest

[Safa Alfulaij]

safaalfulaij added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in renamedialog.cpp:299
> @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how 
> to switch it to rtl). But I agree the code here should account for rtl anyway.
> 
> @ngraham: I think we should stick to the icon naming spec[1], so that it 
> works with themes other than breeze/oxygen; so it has to be go-previous.
> 
> [1] 
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

The interface of Dolphin is RTL-aware, just the panels and the file view are 
not (for many usability reasons)
Make sure to have Qt translations installed if the layout isn't switching 
automatically with the language change.

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, bruns

D22069: Localize long number strings

[Safa Alfulaij]

safaalfulaij added a comment.


  This is great! Thanks!
  Same issue in here: https://phabricator.kde.org/D13219
  I'm not sure if this is the correct way of fixing it, but I think there isn't 
any other prvoided by Qt, other then `QLocale::toString`. Maybe it's the easy 
way here :)

REPOSITORY
  R249 KI18n

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

To: ngraham, #localization, #frameworks, broulik
Cc: safaalfulaij, mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, 
LeGast00n, michaelh, ngraham, bruns

D19785: DocumentPrivate: Review del/backspace

[Safa Alfulaij]

safaalfulaij added a comment.


  Wow! Great :)
  Thanks for looking into this, but I'm afraid that the problem isn't with 
backspace/del, but with the cursor positioning, at least for RTL and mixed text.
  I actually found this to be a problem in Qt after trying to solve it myself 
in KTextEditor: https://bugreports.qt.io/browse/QTBUG-65508
  Not sure what's the root of the problem :(

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor
Cc: safaalfulaij, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

Re: CI system disruption

[Safa Alfulaij]

Thank you for all of your work, Ben!
I hope that you can get to a conclusion to stop using Crowdin and syncing
all the files with almost only one line change “PO-Revision-Date” that does
not even count as a translation work!
This is what happens when teams just do what they want regardless of
anything. (One of the reasons that we didn't continue with the “using
Pootle” idea in the Arabic team.)

Regards,
Safa

‫في الأحد، 6 يناير 2019 في 12:16 م تمت كتابة ما يلي بواسطة ‪Ben Cooksley‬‏
<‪bcooks...@kde.org‬‏>:‬

> Hi all,
>
> Due to a massive translation change which took place in the last 24
> hours, the CI system is currently in the process of rebuilding the
> whole of KDE for all the platforms it covers (both branch sets)
>
> This is a process that has already been underway for several hours,
> and at this stage is expected to take several more. Normal CI service
> will unfortunately be unavailable until this completes.
>
> Unfortunately, as a matter of bad timing, the network card in one of
> the main CI builders has also glitched and as a consequence is now
> intermittent from time to time. This is causing various builds to fail
> as a result. Those builds that do fail will be restarted by Sysadmin
> once these issues have been corrected.
>
> The translation team which made the change in question has been
> requested to contact Sysadmin regarding the large change and it's
> nature.
>
> Apologies for any inconvenience caused.
>
> Regards,
> Ben Cooksley
> KDE Sysadmin
>

D16421: Improve emblem contrast, legibility and consistency

[Safa Alfulaij]

safaalfulaij added a comment.


  mmm, sorry. I saw this today and I just took a minute to understand it. How 
does this looks consistent with the theme, or even nice (really sorry)? Please 
teach me.
  Thank you.
  F6529631: Screenshot_٢٠١٩٠١٠٦_١٣١٠٠٤.png 


REPOSITORY
  R266 Breeze Icons

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

To: ndavis, #vdg, ngraham
Cc: safaalfulaij, bruns, ngraham, bcooksley, kde-frameworks-devel, #vdg, 
michaelh

D15829: make units and prefixes of formatValue translatable

[Safa Alfulaij]

safaalfulaij added inline comments.

INLINE COMMENTS

> kformatprivate.cpp:124
> +{ KFormat::UnitPrefix::Milli, 1e-3,  bpow(-10), tr("m", "SI prefix 
> for 10^⁻3") , QString() },
> +{ KFormat::UnitPrefix::Unity, 1.0,   1.0  , QString()
>   , QString()},
> +{ KFormat::UnitPrefix::Kilo,  1e3,   bpow(10) , tr("k", "SI prefix 
> for 10^3")  , tr("Ki", "IEC binary prefix for 2^10")},

Space before `}`?

> kformatprivate.cpp:125
> +{ KFormat::UnitPrefix::Unity, 1.0,   1.0  , QString()
>   , QString()},
> +{ KFormat::UnitPrefix::Kilo,  1e3,   bpow(10) , tr("k", "SI prefix 
> for 10^3")  , tr("Ki", "IEC binary prefix for 2^10")},
> +{ KFormat::UnitPrefix::Mega,  1e6,   bpow(20) , tr("M", "SI prefix 
> for 10^6")  , tr("Mi", "IEC binary prefix for 2^20") },

Space before `}`?

> kformatprivate.cpp:127
> +{ KFormat::UnitPrefix::Mega,  1e6,   bpow(20) , tr("M", "SI prefix 
> for 10^6")  , tr("Mi", "IEC binary prefix for 2^20") },
> +{ KFormat::UnitPrefix::Giga,  1e9,   bpow(30) , tr("G", "SI prefix 
> for 10^9")  , tr("Gi", "IEC binary prefix for 2^30")  },
> +{ KFormat::UnitPrefix::Tera,  1e12,  bpow(40) , tr("T", "SI prefix 
> for 10^12") , tr("Ti", "IEC binary prefix for 2^40") },

Extra space after `}`?

REPOSITORY
  R244 KCoreAddons

BRANCH
  translate_units

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

To: astippich, bruns, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15829: make units and prefixes of formatValue translatable

[Safa Alfulaij]

safaalfulaij accepted this revision.
safaalfulaij added a comment.
This revision is now accepted and ready to land.


  Thanks!
  I think this solved all my issues. I can for meter unit change the formatting 
so that `tr("%1 %2", "no Prefix")` is translated to `"%1%2"` and choose to keep 
the dot or remove it in the `unitString`s translation. (`"bit"` -> `".bit"`, 
`"m"` -> `"m"`)
  By the way, shouldn't that also be the case for English? I don't see people 
write `"1 m"` but `"1m"`, `"200 B"` but `"200B"`, same for `Hz`, not sure about 
`bit`s. The space is not there when abbreviation is used.

REPOSITORY
  R244 KCoreAddons

BRANCH
  translate_units

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

To: astippich, bruns, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15829: make units and prefixes of formatValue translatable

[Safa Alfulaij]

safaalfulaij added a comment.


  I came into anther problem, this time with the unit selection.
  In Arabic, we don't put a dot between the prefix and the unit if we're 
talking about meters (mm, cm, km), but we do with other units (k.Byte, M.Byte, 
G.Byte).
  
  If ki18n was used here, all of this can be locale-specific, but unfortunately 
that's not the case. I think I understand now why there are many strings for 
all of this unit stuff.

INLINE COMMENTS

> astippich wrote in kformatprivate.cpp:168
> Overlooked that one. Must this then be something like tr("1%2%).arg(... ...)?

Well, I searched other languages and no one seems to do as Arabic does. We do 
not abbreviate in this case and just write the whole "Mebi.Byte" ("Byte" is 
always written, "M.Byte" > "MB").
And of course, there is no one solid naming of the units, so adding just one 
letter will not get the right meaning. Adding another dot will make things more 
crowded -> "M.i.Byte" (This is fine in English, but in Arabic it is really ugly 
and means nothing.)

REPOSITORY
  R244 KCoreAddons

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

To: astippich, bruns, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15829: make units and prefixes of formatValue translatable

[Safa Alfulaij]

safaalfulaij added a comment.


  Since the joining between number and format is translatable, that's great. I 
meant that if it is kept untranslatable, problems regarding RTL will happen.

INLINE COMMENTS

> kformatprivate.cpp:115
>  const PrefixMapEntry map[] = {
> -{ KFormat::UnitPrefix::Yocto, 1e-24, bpow(-80), u'y' },
> -{ KFormat::UnitPrefix::Zepto, 1e-21, bpow(-70), u'z' },
> -{ KFormat::UnitPrefix::Atto,  1e-18, bpow(-60), u'a' },
> -{ KFormat::UnitPrefix::Femto, 1e-15, bpow(-50), u'f' },
> -{ KFormat::UnitPrefix::Pico,  1e-12, bpow(-40), u'p' },
> -{ KFormat::UnitPrefix::Nano,  1e-9,  bpow(-30), u'n' },
> -// Thanks to broken MSVC, we can not use u'µ', but have to use the 
> unicode codepoint
> -{ KFormat::UnitPrefix::Micro, 1e-6,  bpow(-20), QChar(0xB5) },
> -{ KFormat::UnitPrefix::Milli, 1e-3,  bpow(-10), u'm' },
> -{ KFormat::UnitPrefix::Unity, 1.0, 1.0, u'\0' },
> -{ KFormat::UnitPrefix::Kilo,  1e3,   bpow(10), u'k' },
> -{ KFormat::UnitPrefix::Mega,  1e6,   bpow(20), u'M' },
> -{ KFormat::UnitPrefix::Giga,  1e9,   bpow(30), u'G' },
> -{ KFormat::UnitPrefix::Tera,  1e12,  bpow(40), u'T' },
> -{ KFormat::UnitPrefix::Peta,  1e15,  bpow(50), u'P' },
> -{ KFormat::UnitPrefix::Exa,   1e18,  bpow(60), u'E' },
> -{ KFormat::UnitPrefix::Zetta, 1e21,  bpow(70), u'Z' },
> -{ KFormat::UnitPrefix::Yotta, 1e24,  bpow(80), u'Y' },
> +{ KFormat::UnitPrefix::Yocto, 1e-24, bpow(-80), tr("y", "Metric 
> prefix for 10^⁻24") },
> +{ KFormat::UnitPrefix::Zepto, 1e-21, bpow(-70), tr("z", "Metric 
> prefix for 10^⁻21") },

Line 160:

  } else {
  value /= entry->binaryFactor;
  prefixString = QString(entry->prefixChar).toUpper();

How this is “Metric”?

> kformatprivate.cpp:168
>  if (dialect == KFormat::IECBinaryDialect) {
>  prefixString += u'i';
>  }

And what about this? //It should never be a puzzle string by the way.//

REPOSITORY
  R244 KCoreAddons

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

To: astippich, bruns, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15309: [Calendar] Wrap day name index around

[Safa Alfulaij]

safaalfulaij added a comment.


  I thought that this is a feature/cool look :D
  Thanks a lot!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: broulik, #plasma, Zren
Cc: safaalfulaij, kde-frameworks-devel, michaelh, ngraham, bruns

D14733: [KFormat] Add human readable list displaying function

[Safa Alfulaij]

safaalfulaij retitled this revision from "[KFormat] Add human readble list 
displaying function" to "[KFormat] Add human readable list displaying function".

REPOSITORY
  R244 KCoreAddons

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

To: safaalfulaij, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D14733: [KFormat] Add human readble list displaying function

[Safa Alfulaij]

safaalfulaij added a comment.


  In D14733#306325 , @bruns wrote:
  
  > You should start with
  >  a) a use case
  >  b) show it actually matches the needs of different languages.
  >
  > For e.g. englisch and the most european languages, two different positions 
are sufficent. Also, the AndListType and the UnitListType are identical. What 
is missing is a format where no conjunction is used, i.e. all lists elements 
are joined using ', '
  
  
  a) Most of the uses I found is in PIM, where day names and people names are 
joined for a neat sentence. Maybe even email header renderer could use this. 
Other uses: KAlgebra 
 and 
KTurtle . 
Maybe even KMyMoney with the print check function (representing the full text 
representation of the amount.)
  Not a very strong use case, yes, but I thought this can be helpful.
  
  b) I agree with this that most languages use 2 positions only. I prefered to 
do what Unicode is doing 
 
and provide all the three forms they provide, to be in the safe side.
  And you can see in that document all the forms they have. I thought it will 
be an overkill to add them all. I've scanned most languages in Unicode and 
there isn't any difference between normal and shorter versions.
  
  If you see that this could be useful, then great, let's develop it. Otherwise 
we can just ignore it till we find a good use case.

REPOSITORY
  R244 KCoreAddons

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

To: safaalfulaij, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D14733: [KFormat] Add human readble list displaying function

[Safa Alfulaij]

safaalfulaij updated this revision to Diff 39425.
safaalfulaij added a comment.


  Fix test
  No unrelated changes

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14733?vs=39418=39425

BRANCH
  master

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

AFFECTED FILES
  autotests/kformattest.cpp
  autotests/kformattest.h
  src/lib/util/kformat.cpp
  src/lib/util/kformat.h
  src/lib/util/kformatprivate.cpp
  src/lib/util/kformatprivate_p.h

To: safaalfulaij, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D14733: [KFormat] Add human readble list displaying function

[Safa Alfulaij]

safaalfulaij added a comment.


  Tbh I don't fully understand the `struct` and `find` statments (just saw 
working similar code and edited-copy-pasted). I prefer if someone can continue 
this instead of me.

REPOSITORY
  R244 KCoreAddons

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

To: safaalfulaij, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14733: [KFormat] Add human readble list displaying function

[Safa Alfulaij]

safaalfulaij created this revision.
safaalfulaij added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
safaalfulaij requested review of this revision.

REVISION SUMMARY
  Add formatList() to convert QStringLists to human readable strings.
  This makes it possible to display strings like "A, B, C and D" natively in
  all languages.

TEST PLAN
  Added a test

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

AFFECTED FILES
  autotests/kformattest.cpp
  autotests/kformattest.h
  src/lib/util/kformat.cpp
  src/lib/util/kformat.h
  src/lib/util/kformatprivate.cpp
  src/lib/util/kformatprivate_p.h

To: safaalfulaij, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13584: KFormat: Replace byte specific implementation with generic one

[Safa Alfulaij]

safaalfulaij added a comment.


  Can we please take a look at this  
before continuing here?

REPOSITORY
  R244 KCoreAddons

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

To: bruns, #frameworks
Cc: safaalfulaij, dfaure, astippich, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13583: KFormat: Allow usage of quantities beyond bytes and seconds

[Safa Alfulaij]

safaalfulaij added a comment.


  > Yes. As you said, these are symbols. For the names there may be 
transliterations, the symbols stay the same, to avoid ambiguities. This also 
matches my experience with datasheets, the text may be chinese, but 
measurements are given in SI units.
  
  I just saw¹ this PR and I'd like to say something.
  Yes, in scientific papers we use English terms/symbols so that they are 
understood by others, and to have a system for communication (same goes for 
IUPAC naming.)
  But for users this is wrong, especially with different scripts, and even with 
right-to-left languages. We are here so that they understand that, not others.
  These are the Chinese 
, Russian 
, Arabic 
, Hebrew 
 and Persian 
 Unicode pages 
showing different units.
  
  Maybe in Chinese you don't use the localized version of the symbols, but I 
think you use English full-width characters, right? How these will be localized 
then?
  Same case goes for Russian that uses thier cyrillic script.
  
  Arabic, Hebrew and Persian are a different case. It is always considered bad 
to mix between Arabic and English text, and if so then in extreme hard cases 
(where you can't translate MySQL or a command-line application that no one 
really use except for developers, or maybe Qt method names or classes.).
  But here we are mixing just few characters with few numbers and things are in 
a mess. If this was used in QML (where text direction is detected 
automaticlly), things will go wrong for RTL languages, and you'll see the unit 
name on the right and the quantity on the left.
  
  F6180270: Screenshot_٢٠١٨٠٨٠٧_٢٣٥٨٠٩.png 

  //Fixed by prepending a RLM to the sentence, as Kate detects text direction 
automatically as well//
  
  Please, if your language uses English symbols, it doesn't mean that every 
other must follow. KI18n previous (and maybe current, not sure) developers 
created scripting-enabled translation system so that each language team can do 
whatever suits thier language.
  
  Thank you.
  
  1: After I saw the new PR  using this.
  
  P.S: Check these files in KDE l10n repositories:
  
https://websvn.kde.org/trunk/l10n-kf5/fr/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 225)
  
https://websvn.kde.org/trunk/l10n-kf5/ru/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 230)
  
https://websvn.kde.org/trunk/l10n-kf5/fa/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 325)
  
https://websvn.kde.org/trunk/l10n-kf5/he/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 247)
  
https://websvn.kde.org/trunk/l10n-kf5/ar/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 204)

REPOSITORY
  R244 KCoreAddons

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

To: bruns, #frameworks, astippich
Cc: safaalfulaij, siddharthasahu, bcooksley, kossebau, kde-frameworks-devel, 
astippich, michaelh, ngraham, bruns

D14565: Fix overflow in rounding code

[Safa Alfulaij]

safaalfulaij accepted this revision.
safaalfulaij added a comment.
This revision is now accepted and ready to land.


  Worked great, thanks!
  Maybe you should use `largeValue` instead of `largevalue`.
  But please take an approval from a developer, it worked for me but I have no 
idea if it can cause any trouble :)

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: svuorela, cfeck, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14345: Give the PlasmaComponents3 TextField the ability to have a Clear button

[Safa Alfulaij]

safaalfulaij added a comment.


  In D14345#297340 , @ngraham wrote:
  
  > 1. Does this mean that PlasmaComponents is semi or fully deprecated or 
"legacy", and we should be porting Plasma stuff to Kirigami instead?
  > 2. Since there's no Kirigami `TextField`, what do we do with this patch? Is 
there any reason why we can't improve the PC3 `TextField` to match the features 
that the PC2 `TextField` has? Or should we create a Kirigami `TextField` 
instead and port current PC2/3 clients to use it?
  
  
  https://cgit.kde.org/kirigami.git/tree/src/controls/Label.qml#n45
  
  I'll take the liberty and say that we just need to use QQC2 directly, and it 
will be themed correctly with all the enviroment variable tricks in Plasma :)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, mart, davidedmundson
Cc: safaalfulaij, kde-frameworks-devel, michaelh, ngraham, bruns

D14345: Give the PlasmaComponents3 TextField the ability to have a Clear button

[Safa Alfulaij]

safaalfulaij added a comment.


  Great! Now I know why the port to PC3 didn't start :D
  Now to the patch. First, by this we will allow the user to get text under the 
icon, which will be impossible to read.
  Second is what I'm here for :)
  The clear button placement and icon should not follow the locale, but the 
actual text imh. You can write Arabic in an English system, and vice versa.
  
  I've made this simple file to explain my point:
  
import QtQuick 2.7
import QtQuick.Controls 2.3
import QtQuick.Layouts 1.3

import org.kde.plasma.core 2.0 as PlasmaCore

Item {
width: 300
height: 200

LayoutMirroring.enabled: true // This value (true or false) shouldn’t 
affect anything regarding the Clear button!
LayoutMirroring.childrenInherit: true

Frame {
anchors.centerIn: parent
width: 200
RowLayout {
LayoutMirroring.enabled: false
anchors.fill: parent
layoutDirection: textInput.isRightToLeft(0, textInput.length) ? 
Qt.RightToLeft : Qt.LeftToRight
TextField {
id: textInput
Layout.fillWidth: true
}
AbstractButton {
contentItem: PlasmaCore.IconItem {
source: textInput.isRightToLeft(0, textInput.length) ? 
"edit-clear-locationbar-ltr" : "edit-clear-locationbar-rtl"
}
}
}
}
}
  
  Whatever the locale is, or the mirroring is, things inside the `Frame` 
shouldn't change (well, this won't work fully as the context menu might not get 
rendered correctly, but I didn't want to write 4 `if` statments in the 
`rightMargin` and `leftMargin` properties, as you can't set it to be absolute. 
With mirroring, right is treated as left, and vice versa)
  
  Results:
  F6148886: Screenshot_٢٠١٨٠٧٢٦_٢٣٥٤١٤.png 

  
  F6148887: Screenshot_٢٠١٨٠٧٢٦_٢٣٥٤٢٢.png 


REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, mart, davidedmundson
Cc: safaalfulaij, kde-frameworks-devel, michaelh, ngraham, bruns

D12130: Use the more user-friendly string "File type" in the save dialogs

[Safa Alfulaij]

safaalfulaij added a comment.


  Btw, the `qt` tag is from Qt3 days. I'm not sure if it must be richtext, if 
so then a simple `html` or even KUIT markups are better choice :)
  
  Not sure about string freeze though..

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #vdg, bruns, alexeymin, rkflx, abetts
Cc: safaalfulaij, davidc, ltoscano, cfeck, rkflx, alexeymin, abetts, bruns, 
michaelh, ngraham

D10592: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij accepted this revision.
safaalfulaij added a comment.
This revision is now accepted and ready to land.


  Now since I'm a reviewer, sure :)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  rtl

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

To: mvourlakos, #plasma, mart, safaalfulaij
Cc: safaalfulaij, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10592: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In D10592#209670 , @mvourlakos 
wrote:
  
  > @safaalfulaij is there a green light for this?
  
  
  Sure. Go ahead and commit.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10592: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added inline comments.

INLINE COMMENTS

> ComboBox.qml:113
>  highlightMoveDuration: 0
> -LayoutMirroring.enabled: true
> +// HACK: When the ComboBox is not inside the base Item, it does 
> not inherit
> +// the LayoutMirroring options. This is a workaround to fix this 
> by enforcing

When the ComboBox is not inside a top-level Window, it's Popup does not inherit

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  Reported: https://bugreports.qt.io/browse/QTBUG-66446
  Although this is really unlikly to happen, a widget having a combobox, but 
this is the case of Latte where the whole config overlay is just a `MouseArea`.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, michaelh, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In D9275#207623 , @mvourlakos wrote:
  
  > Can you build Latte from master version? All comboboxes in its settings 
window are PlasmaComponents3
  
  
  :)
  Ok, I can assume that Latte doens't have a `Window`/`ApplicationWindow`, and 
that is why this is needed.
  I've tried this and it didn't work as with `ApplicationWindow` instead of the 
base `Item`:
  
import QtQuick 2.7
import QtQuick.Layouts 1.3

import "plasma" as Plasma

Item {
LayoutMirroring.enabled: true
LayoutMirroring.childrenInherit: true

width: 350
height: 150

RowLayout {
anchors.fill: parent
Plasma.ComboBox {
id: combo
model: ["أولا_", "Second_", "Third"]
}
}
}
  
  We should include this for now, with the HACK comment, till we can get it 
somehow solved in Qt.
  Sorry for all the misunderstandings!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, michaelh, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  I've done some more testing and read some Qt docs. `Popup` item get 
reparented to the window once displayed, so LayoutMirroring is applied to it 
only at that point (applying it to the contentItem).
  So my comments above about `Popup` not inheerting `Item` are wrong, sorry.
  I'm still not sure if this is needed really.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, michaelh, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In D9275#207435 , @mvourlakos wrote:
  
  > > Would you please add a HACK so that we know when to remove this?
  >
  > You mean as a comment ?
  
  
  Yes, preferably with the Qt bug number and some explanation.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, michaelh, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In D9275#207260 , @mvourlakos wrote:
  
  > @mart I fixed this issue by changing these lines to:
  >
  >   113LayoutMirroring.enabled: Qt.application.layoutDirection 
=== Qt.RightToLeft
  >   114LayoutMirroring.childrenInherit: true
  >
  
  
  Would you please add a HACK so that we know when to remove this? This is 
because `Popup` doesn't inherit `LayoutMirroring` and bases the direction 
according to the locale used, exactly as in the Qt bug report I've mentioned 
above.
  
  In normal conditions, where language = locale, this is not needed. But in 
cases where they are different, the locale direction is preffered for `Popup`, 
and therfore this will be needed.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, michaelh, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added inline comments.

INLINE COMMENTS

> ComboBox.qml:113
>  highlightMoveDuration: 0
> +LayoutMirroring.enabled: true
> +LayoutMirroring.childrenInherit: true

So it is true even for LTR locales?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D9275#190606, @mvourlakos wrote:
  
  > To test it in Latte I added in the head of 
/shell/package/contents/configuration/AppearanceConfig.qml
  >
  >   import org.kde.plasma.components 3.0 as PlasmaComponents3
  >
  >
  > and I set up the ComboBox in that file to:
  >
  >   PlasmaComponents3.ComboBox
  >
  >
  >
  >
  > > But I can't understand how changing this V3 solved V2..
  >
  > V2 components are based totally at Qt Styles, V3 are reimplementing the 
behavior
  >  PlasmaComponents2.ComboBox is a ComboBoxStyle  but
  >  PlasmaComponents3.ComboBox is a real ComboBox
  
  
  Did you test this with your LTR layout with only `--reverse`?
  I can say now that the popup box (that is a `Popup`) takes its direction from 
the locale used and not from the application, since it doesn't inhert `Item`, 
and `LayoutMirroring` doesn't affect it
  
  It is similar to this  issue 
with the `Slider`.
  I'll add more info to that soon.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D9275#190569, @mvourlakos wrote:
  
  > with your example me I cant reproduce it either...
  >  I can only reproduce it with Latte Settings window, all other components 
are aligned correctly except the PlasmaComponent3.Combobox list items.
  >
  > you are right, this might be a downstream bug...
  >
  > Do you want to revert it 
  >  I can do it
  
  
  I checked out the source of latte-dock and it seems that it is using the old 
QQC1-based controls, which are problematic, especially with RTL :)
  This is a change for QQC2-based controls (version 3.0 of 
org.kde.plasma.components).
  
  But I can't understand how changing this V3 solved V2..

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D9275#190497, @mvourlakos wrote:
  
  > how did you test it?
  >
  > I tried in an qml app (Latte dock) by passing the parameter "--reverse"...
  >  If your system is already using in RTL language, have you tried with 
--reverse?
  
  
  I am using this with qmlsence-qt5.
  `plasma` folder is the ones before this, and `plasmacomponents3` is the one 
after this.
  Using C locale, everything aligned to left. Using --reverse will align 
everything to right.
  Using ar locale, everything aligned to right. Using --reverse will **not** 
reverse it to LTR layout and it makes everything aligned to left.
  
  Can't see the problem really.
  
import QtQuick 2.7
import QtQuick.Layouts 1.3
import QtQuick.Controls 2.0
import "plasma" as Plasma
import "plasmacomponents3" as PlasmaNEW

ApplicationWindow {
LayoutMirroring.enabled: Qt.application.layoutDirection == 
Qt.RightToLeft
LayoutMirroring.childrenInherit: true
RowLayout {
Plasma.Button {
id: fileButton
text: "File"
onClicked: menu.open()

Plasma.Menu {
id: menu
y: fileButton.height
x: fileButton.width - width

Plasma.MenuItem {
text: "New..."
}
Plasma.MenuItem {
text: "Open..."
}
Plasma.MenuItem {
text: "Save"
}
}
}

Plasma.Label {
text: "2"
}
Plasma.ComboBox {
model: ["أولا_", "Second_", "Third"]
}
PlasmaNEW.ComboBox {
model: ["أولا_", "Second_", "Third"]
}
Plasma.ProgressBar {
value: 0.3
}
Plasma.Switch {
text: "Switch"
}
}
}

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9275: fix RTL appearance for ComboBox

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D9275#190464, @mvourlakos wrote:
  
  > In https://phabricator.kde.org/D9275#190441, @safaalfulaij wrote:
  >
  > > Can you please explain why these are needed? I tested 
 plasmacomponents3 and the combobox was 
working correctly.
  >
  >
  > It wasnt, use english text in the list items to see the issue without the 
patch.
  
  
  Nope, I have also English text in the model. Its:
  تجربة_
  test_
  test

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, mart
Cc: safaalfulaij, mart, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D9793: Fall back to language name for translations lookup if locale name fails

[Safa Alfulaij]

safaalfulaij added a comment.


  > Why is this an issue?
  >  There's no difference really in loading ar/LC_MESSAGES/xxx.qm and 
LC_MESSAGES/xxx_ar.qm (or something like that), i.e. you would have the same 
problem if all translations would be in the same folder.
  
  Well, we were to simplify this to one call of loadTranslation, but yes, not a 
big deal.
  
  > The only relevant things I can see is that it replaces all occurences of 
'-' with '_' (which is necessary only because it gets the languages from 
QLocale::uiLanguages()), and that it doesn't cut off at the first '_', but 
creates entries cut off at every one. (i.e. "de_XX_YY" would yield "de_XX_YY", 
"de_XX" and "de" to try, IIUIC, while the current patch would only try 
"de_XX_YY" and "de")
  > 
  > I could do something similar, i.e. lookup translations in a while loop and 
cut off at the right-most '_' if a lookup fails until it succeeds.
  
  Well, that would be great for an ideal world. We can live for now and check 
the locales that KDE is currently translated to, and just adapt to them.
  
  > The current code does get the locale/language from Qt anyway. (it calls 
QLocale::system(), which does respect LANG and LANGUAGE )
  > 
  > It probably would also be a good idea to get a list of languages via 
QLocale::uiLanguages() instead like that find_translation() function does (that 
would also support things like LANGUAGE="de:ar"). But that's unrelated to this 
fix IMHO.
  
  Maybe later, I like full concept implementations, but yes, not needed.
  
  This will be enough for the time being. Thanks! :-)

REPOSITORY
  R240 Extra CMake Modules

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

To: wbauer, #frameworks
Cc: safaalfulaij, #build_system

D9793: Fall back to language name for translations lookup if locale name fails

[Safa Alfulaij]

safaalfulaij added a comment.


  I went through Qt code, as Qt applications are opened with my language 
correctly where KF ones (those with QM) don't.
  The whole issue is that we have each locale's translations in a separate 
folder (`ar/LC_MESSAGES`, `en/LC_MESSAGES`, `de/LC_MESSAGES`, etc.)
  Qt uses internally `find_translation` 

 which goes through a list of guesses to find the correct language, according 
to the LANGUAGE preferences and the locale itself.
  Maybe we can borrow that code and utilize it, that would be much better.
  
  Thanks for opening this, @wbauer!

REPOSITORY
  R240 Extra CMake Modules

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

To: wbauer, #frameworks
Cc: safaalfulaij, #build_system

Re: Problem in loading QM translations with LANGUAGE="" envar

[Safa Alfulaij]

Systemsettings sets that, and there is no ar_BH language, but ar only.
That's the standard for Arabic translations.

On Jan 10, 2018 10:09 PM, "Albert Astals Cid" <aa...@kde.org> wrote:

> El dimecres, 10 de gener de 2018, a les 5:49:54 CET, Safa Alfulaij va
> escriure:
> > Well, I don't want to remove it but it's a must. First it's not right
> > logically and second is that Qt prefers it over LANG, and use any locale
> > varient since it has no country name. (QLocale("ar") returns "Arabic
> > (Egypt)" (which is defind in unicode), instead of my "Arabic (Bahrain)"
> > locale.
> > And it's the only thing that is not getting translated which is
> obviously a
> > bug.
>
> Why not use ar_BH as LANGUAGE?
>
> this is the output of my locale
>
> $ locale
> LANG=ca_ES.UTF-8
> LANGUAGE=ca_ES
> LC_CTYPE="ca_ES.UTF-8"
> LC_NUMERIC="ca_ES.UTF-8"
> LC_TIME="ca_ES.UTF-8"
> LC_COLLATE="ca_ES.UTF-8"
> LC_MONETARY="ca_ES.UTF-8"
> LC_MESSAGES="ca_ES.UTF-8"
> LC_PAPER="ca_ES.UTF-8"
> LC_NAME="ca_ES.UTF-8"
> LC_ADDRESS="ca_ES.UTF-8"
> LC_TELEPHONE="ca_ES.UTF-8"
> LC_MEASUREMENT="ca_ES.UTF-8"
> LC_IDENTIFICATION="ca_ES.UTF-8"
> LC_ALL=
>
> Cheers,
>   Albert
>
>
> >
> > Regards
> >
> > On Jan 10, 2018 12:20 AM, "Albert Astals Cid" <aa...@kde.org> wrote:
> > > El dimarts, 9 de gener de 2018, a les 20:08:02 CET, Safa Alfulaij va
> > >
> > > escriure:
> > > > Hello all.
> > > >
> > > > After doing some investigation about a problem, I came into an issue
> in
> > > > loading correct translations in Frameworks (that has the “_qt”
> suffix in
> > > > translation system).
> > > > Previously, I had my LANGUAGE enviroment variable containing “ar”,
> which
> > >
> > > is
> > >
> > > > because I added Arabic in my “Language” KCM (without knowing that I
> > > > don't
> > > > need it if the locale is alread set to Arabic).
> > > > This was normal and things was translated well. Today I removed that
> > > > “ar”
> > > > because it's not needed, and all applications (Qt, KDE, Gnome, other
> > >
> > > KF5s)
> > >
> > > > worked except for those frameworks which are using QM files (the ones
> > >
> > > with
> > >
> > > > “_qt” suffix). The translation isn't loaded and strings are shown in
> > > > English.
> > >
> > > So don't remove it?
> > >
> > > Cheers,
> > >
> > >   Albert
>
>
>

Re: Problem in loading QM translations with LANGUAGE="" envar

[Safa Alfulaij]

More investigation shows that it does not work with other locales than the
default one (the one that is used in case you don't pass a country code,
which, in case of Arabic, is ar_EG).

[safa@archlinux]$ LANG=ar_BH.utf8
> [safa@archlinux]$ python testQtLocale.py
> b'\xd9\xa2\xd9\xa0 B'
> [safa@archlinux]$ LANG=ar_EG.utf8
> [safa@archlinux]$ python testQtLocale.py
> b'\xd9\xa2\xd9\xa0 \xd8\xa8\xd8\xa7\xd9\x8a\xd8\xaa'
> [safa@archlinux]$ LANG=ar_SA.utf8
> [safa@archlinux]$ python testQtLocale.py
> b'\xd9\xa2\xd9\xa0 B'
> [safa@archlinux]$ LANG=ar_TN.utf8
> [safa@archlinux]$ python testQtLocale.py
> b'20 B'


Notice the translated version of "B" in ar_EG. The locale is set correctly
as we can see ar_TN uses the correct English/Arabic (not Hindi) numbering
system.
So it seems that the files are registered for that ar_EG locale only, which
is causing the trouble.

Note: Qt prioritizes LANGUAGE which has no country code, and that was the
problem. I'll be sending a small patch to use LANG if the first choice in
LANGUAGE is the same as LANG's main language.

Regards

2018-01-10 5:49 GMT+03:00 Safa Alfulaij <safa1996alful...@gmail.com>:

> Well, I don't want to remove it but it's a must. First it's not right
> logically and second is that Qt prefers it over LANG, and use any locale
> varient since it has no country name. (QLocale("ar") returns "Arabic
> (Egypt)" (which is defind in unicode), instead of my "Arabic (Bahrain)"
> locale.
> And it's the only thing that is not getting translated which is obviously
> a bug.
>
> Regards
>
> On Jan 10, 2018 12:20 AM, "Albert Astals Cid" <aa...@kde.org> wrote:
>
>> El dimarts, 9 de gener de 2018, a les 20:08:02 CET, Safa Alfulaij va
>> escriure:
>> > Hello all.
>> >
>> > After doing some investigation about a problem, I came into an issue in
>> > loading correct translations in Frameworks (that has the “_qt” suffix in
>> > translation system).
>> > Previously, I had my LANGUAGE enviroment variable containing “ar”,
>> which is
>> > because I added Arabic in my “Language” KCM (without knowing that I
>> don't
>> > need it if the locale is alread set to Arabic).
>> > This was normal and things was translated well. Today I removed that
>> “ar”
>> > because it's not needed, and all applications (Qt, KDE, Gnome, other
>> KF5s)
>> > worked except for those frameworks which are using QM files (the ones
>> with
>> > “_qt” suffix). The translation isn't loaded and strings are shown in
>> > English.
>>
>> So don't remove it?
>>
>> Cheers,
>>   Albert
>>
>

Re: Problem in loading QM translations with LANGUAGE="" envar

[Safa Alfulaij]

Well, I don't want to remove it but it's a must. First it's not right
logically and second is that Qt prefers it over LANG, and use any locale
varient since it has no country name. (QLocale("ar") returns "Arabic
(Egypt)" (which is defind in unicode), instead of my "Arabic (Bahrain)"
locale.
And it's the only thing that is not getting translated which is obviously a
bug.

Regards

On Jan 10, 2018 12:20 AM, "Albert Astals Cid" <aa...@kde.org> wrote:

> El dimarts, 9 de gener de 2018, a les 20:08:02 CET, Safa Alfulaij va
> escriure:
> > Hello all.
> >
> > After doing some investigation about a problem, I came into an issue in
> > loading correct translations in Frameworks (that has the “_qt” suffix in
> > translation system).
> > Previously, I had my LANGUAGE enviroment variable containing “ar”, which
> is
> > because I added Arabic in my “Language” KCM (without knowing that I don't
> > need it if the locale is alread set to Arabic).
> > This was normal and things was translated well. Today I removed that “ar”
> > because it's not needed, and all applications (Qt, KDE, Gnome, other
> KF5s)
> > worked except for those frameworks which are using QM files (the ones
> with
> > “_qt” suffix). The translation isn't loaded and strings are shown in
> > English.
>
> So don't remove it?
>
> Cheers,
>   Albert
>

Problem in loading QM translations with LANGUAGE="" envar

[Safa Alfulaij]

Hello all.

After doing some investigation about a problem, I came into an issue in
loading correct translations in Frameworks (that has the “_qt” suffix in
translation system).
Previously, I had my LANGUAGE enviroment variable containing “ar”, which is
because I added Arabic in my “Language” KCM (without knowing that I don't
need it if the locale is alread set to Arabic).
This was normal and things was translated well. Today I removed that “ar”
because it's not needed, and all applications (Qt, KDE, Gnome, other KF5s)
worked except for those frameworks which are using QM files (the ones with
“_qt” suffix). The translation isn't loaded and strings are shown in
English.

I've made this very small Python script to test it. Notice the untranslated
B (Bytes) and the translated one (بايت). The number 20 (in result is ٢٠) is
localized correctly using the QLocale class, meaning that the locale is set
properly.

printvar LANGUAGE
Result >>> ''

BEGIN PYTHON3 CODE:

> from PyQt5.QtCore import *
> from PyQt5.QtWidgets import *
> from PyKF5 import KCoreAddons
> app = QApplication([""])
> format = KCoreAddons.KFormat()
> format.formatByteSize(20)
> Result >>> '٢٠ B'

END PYTHON3 CODE

LANGUAGE=ar

BEGIN PYTHON3 CODE:

> from PyQt5.QtCore import *
> from PyQt5.QtWidgets import *
> from PyKF5 import KCoreAddons
> app = QApplication([""])
> format = KCoreAddons.KFormat()
> format.formatByteSize(20)
> Result >>> '٢٠ بايت'

END PYTHON3 CODE


Regards,
Safa

P.S.: I am not subscribed to the list, please CC me.

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:0f2335f3edc0: Fix compilation (authored by safaalfulaij).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=24349=24361

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/document/katedocument.cpp

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann, cullmann
Cc: bcooksley, brauch, mwolff, ngraham, anthonyfieroni, cullmann, jgrulich, 
dhaumann, hein, kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:c9b412d40334: Fix a regression caused by changing 
backspace key behavior (authored by safaalfulaij).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=21916=24338

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann, cullmann
Cc: mwolff, ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, hein, 
kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij added a comment.


  I can commit but it is showing as not accepted.
  So I'll just wait for the final confirmation.

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann, mwolff, cullmann
Cc: mwolff, ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, hein, 
kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij updated this revision to Diff 21916.
safaalfulaij added a comment.


  - Applying last comment (hopefully I did what you meant :!)

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=19781=21916

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann
Cc: ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, hein, kwrite-devel, 
#frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D7660#158479, @ngraham wrote:
  
  > @safaalfulaij, any progress on the latest comments?
  
  
  Not really. I was busy with college. I'll try finishing it in the coming week.

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann
Cc: ngraham, anthonyfieroni, cullmann, jgrulich, dhaumann, hein, kwrite-devel, 
#frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij updated this revision to Diff 19781.
safaalfulaij added a comment.


  - Typos and better if statement structure

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=19370=19781

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij, #ktexteditor, jgrulich, hein, dhaumann
Cc: jgrulich, dhaumann, hein, kwrite-devel, #frameworks

D7840: Fix trailing space visualization for RTL lines.

[Safa Alfulaij]

safaalfulaij added a comment.


  Sorry, my mistake. The post-commit trigger thing didn't close this, maybe 
because the #post part is there /me how come I didn't notice! :|
  
https://cgit.kde.org/ktexteditor.git/commit/?id=bb328146ecd64f97710eb09f568001cd4d40f622

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, cullmann
Cc: cullmann, kfunk, #frameworks, sars, dhaumann

D7840: Fix trailing space visualization for RTL lines.

[Safa Alfulaij]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:bb328146ecd6: Fix trailing space visualization for RTL 
lines. (authored by safaalfulaij).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7840?vs=19564=19568

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

AFFECTED FILES
  src/render/katerenderer.cpp

To: safaalfulaij, #ktexteditor, cullmann
Cc: cullmann, kfunk, #frameworks, sars, dhaumann

D7840: Fix trailing space visualization for RTL lines.

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D7840#146110, @kfunk wrote:
  
  > Indeed, thanks for those fixes. /me can see a nice blog post coming a 
long...? :)
  
  
  Not yet maybe
  There are other visualizations that are not working for RTL lines, like 
non-printable spaces, non-breakable space and tabs. (Indent lines are not 
needed for RTL lines)
  They don't appear at all. I tried fixing them but didn't succeed :p

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, cullmann
Cc: cullmann, kfunk, #frameworks, sars, dhaumann

D7840: Fix trailing space visualization for RTL lines.

[Safa Alfulaij]

safaalfulaij added a comment.


  Before:
  F3912046: Screenshot_٢٠١٧٠٩١٥_١٧٥٣٥٦.png 

  
  After:
  F3912047: Screenshot_٢٠١٧٠٩١٥_١٧٥٦٢٨.png 


REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor
Cc: cullmann, kfunk, #frameworks, sars, dhaumann

D7840: Fix trailing space visualization for RTL lines.

[Safa Alfulaij]

safaalfulaij created this revision.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Fix trailing space visualization for RTL lines.
  It was getting paint on top of the character (to the right of the cursor).

TEST PLAN
  Tested and worked for both lines

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/render/katerenderer.cpp

To: safaalfulaij, #ktexteditor
Cc: #frameworks, cullmann, sars, dhaumann

D7839: Fix trailing space visualization for RTL lines

[Safa Alfulaij]

safaalfulaij abandoned this revision.

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij
Cc: kwrite-devel, #frameworks, cullmann, sars, dhaumann

D7839: Fix trailing space visualization for RTL lines

[Safa Alfulaij]

safaalfulaij created this revision.
safaalfulaij added a project: Frameworks.
Restricted Application added a project: Kate.

REVISION SUMMARY
  Fix trailing space visualization for RTL lines.

TEST PLAN
  N/A

REPOSITORY
  R39 KTextEditor

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/render/katerenderer.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij
Cc: kwrite-devel, #frameworks, cullmann, sars, dhaumann

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D7660#144736, @jgrulich wrote:
  
  > The bug report is not publicly available, but it was about kate not 
removing composed characters, while other software do that. I now checked this 
with LibreOffice and QtCreator and it seems to remove composed character only 
when you use DELETE and not BACKSPACE. Not sure what is correct, but having 
this as an option seems to be a good idea.
  
  
  Yes that's what is correct, and that's why I didn't add an if statment for 
changing the behaviour in case you used DELETE; because the “Indic” way is the 
correct way for Arabic as well, and that's what is used in normal Qt/GTK 
widgets, both for DELETE and BACKSPACE.

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, jgrulich, hein
Cc: jgrulich, dhaumann, hein, kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij added a comment.


  In https://phabricator.kde.org/D7660#144706, @jgrulich wrote:
  
  > Not sure I can say much, I wrote this patch because I got a bug report 
where for Indic locales they were not able to remove composed characters. while 
other software (QtCreator, Libreoffice) worked pretty well. Also moving cursor 
to the left or to the right moved it by one composed character which I assumed 
should be same behaviour also for backspace/delete so I used same methods to 
identify one exact character which should be removed.
  
  
  Well, I just checked both QtCreator and LibreOffice (using a text from 
Wikipedia) and both gave me different results than Kate currently doing. The 
results are the normal results that Arabic uses.
  I used them with thier defaults, so if there is any option hidden somewhere, 
then it's the same case I'm trying to achieve here.
  Where is that bug report?

REPOSITORY
  R39 KTextEditor

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

To: safaalfulaij, #ktexteditor, jgrulich, hein
Cc: jgrulich, dhaumann, hein, kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij updated this revision to Diff 19370.
safaalfulaij added a comment.


  - Fix global and local view values

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=19368=19370

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij, #ktexteditor, jgrulich, hein
Cc: jgrulich, dhaumann, hein, kwrite-devel, #frameworks

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij updated this revision to Diff 19368.
safaalfulaij added a comment.


  - Add the implementation of backspaceRemoveComposed

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7660?vs=19097=19368

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij, #ktexteditor, jgrulich, hein
Cc: jgrulich, dhaumann, hein, kwrite-devel, #frameworks

D7715: [Plasma Components 3] Fix RTL in some widgets.

[Safa Alfulaij]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:940adc7e6e69: [Plasma Components 3] Fix RTL in some 
widgets. (authored by safaalfulaij).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7715?vs=19255=19351

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/ProgressBar.qml
  src/declarativeimports/plasmacomponents3/RangeSlider.qml
  src/declarativeimports/plasmacomponents3/Slider.qml

To: safaalfulaij, #plasma, #frameworks, mart
Cc: mart, broulik, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D7715: [Plasma Components 3] Fix RTL in some widgets.

[Safa Alfulaij]

safaalfulaij added a comment.


  It seems that the `TabBar`'s problem is an upstream issue with `ListView` and 
highlighting animation.
  Will report about it soon.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: safaalfulaij, #plasma, #frameworks, mart
Cc: mart, broulik, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D7715: [Plasma Components 3] Fix RTL in some widgets.

[Safa Alfulaij]

safaalfulaij retitled this revision from "[WIP] Fix RTL in some 
plasmacomponents3 widgets." to "[Plasma Components 3] Fix RTL in some widgets.".

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: safaalfulaij, #plasma, #frameworks, mart
Cc: mart, broulik, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D7715: [WIP] Fix RTL in some plasmacomponents3 widgets.

[Safa Alfulaij]

safaalfulaij added inline comments.

INLINE COMMENTS

> broulik wrote in ProgressBar.qml:33
> That's pretty clever, actually,
> 
> > A negative scale causes the item to be mirrored when rendered.
> 
> However, not sure we can just blatantly mirror that graphic

Actually, it’s used in default QQC2 styling and I just stole it from there..
http://code.qt.io/cgit/qt/qtquickcontrols2.git/tree/src/imports/controls/Slider.qml#n78

REPOSITORY
  R242 Plasma Framework (Library)

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

To: safaalfulaij, #plasma, #frameworks
Cc: broulik, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D7715: [WIP] Fix RTL in some plasmacomponents3 widgets.

[Safa Alfulaij]

safaalfulaij added a comment.


  One widget I didn't know how to fix is the TabBar (The blue line that flows 
from left to right for the first item.)
  Other widgets are behaving the same as QQC2 ones, like BusyIndicator and 
Dial, so I didn't change them. Not sure if they should be flipped or not.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: safaalfulaij, #plasma, #frameworks
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D7715: [WIP] Fix RTL in some plasmacomponents3 widgets.

[Safa Alfulaij]

safaalfulaij created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Some widgets are missed up in RTL mode, this should fix it.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/ProgressBar.qml
  src/declarativeimports/plasmacomponents3/RangeSlider.qml
  src/declarativeimports/plasmacomponents3/Slider.qml

To: safaalfulaij, #plasma, #frameworks
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij added a reviewer: KTextEditor.

REPOSITORY
  R39 KTextEditor

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

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

D7660: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij created this revision.
Restricted Application added subscribers: Frameworks, kwrite-devel.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  This is a patch trying to fix the regression caused by an old patch 
.
  Backspace key behavior in all applications acts by removing the diacritics 
 one by one and then 
  remove the base character.
  For Indic locales this doesn't work, and the whole character should be 
removed. 
  This causes a problem in Arabic where we just want to remove the diacritic 
and 
  not the whole composed character.
  
  This patch adds a configuration option that is disabled by default. It's 
purpose 
  is to switch between the two modes.
  Enabling it will use the Indic mode, because the default all software uses is 
the 
  normal mode.
  For the delete key, the correct way for Arabic is to remove the diacritics as 
  well, so there is no if statement for it.

TEST PLAN
  KTextEditor compiled normally. Both modes works as expected. No problems 
reported 
  about the test while compiling, not sure if it's working right or not.
  (I copied my changes from an older version source code (matching my current 
  configiration). There might be things I missed when doing that.)

REPOSITORY
  R39 KTextEditor

BRANCH
  master

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

To: safaalfulaij
Cc: kwrite-devel, #frameworks

D7659: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij abandoned this revision.

REPOSITORY
  R39 KTextEditor

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

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

D7659: Fix a regression caused by changing backspace key behavior

[Safa Alfulaij]

safaalfulaij created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This is a patch trying to fix the regression caused by an old patch 
.
  Backspace key behavior in all applications acts by removing the diacritics 
 one by one and then remove the base 
character.
  For Indic locales this doesn't work, and the whole character should be 
removed. This causes a problem in Arabic where we just want to remove the 
diacritic and not the whole composed character.
  
  This patch adds a configuration option that is disabled by default. It's 
purpose is to switch between the two modes.
  Enabling it will use the Indic mode, because the default all software uses is 
the normal mode.
  For the delete key, the correct way for Arabic is to remove the diacritics as 
well, so there is no if statement for it.

TEST PLAN
  KTextEditor compiled normally. Both modes works as expected. No problems 
reported about the test while compiling, not sure if it's working right or not.
  (I copied my changes from an older version source code (matching my current 
configiration). There might be things I missed when doing that.)

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  autotests/src/katedocument_test.cpp
  src/dialogs/katedialogs.cpp
  src/dialogs/navigationconfigwidget.ui
  src/document/katedocument.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h

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