[WIP] Refactor KisNodeDelegate and support RTL
ClosedPublic

Authored by safaalfulaij on Sep 18 2018, 1:56 PM.

Diff Detail

Repository
R37 Krita
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5799
Build 5817: arc lint + arc unit
safaalfulaij created this revision.Sep 18 2018, 1:56 PM
Restricted Application added a project: Krita. · View Herald TranscriptSep 18 2018, 1:56 PM
safaalfulaij requested review of this revision.Sep 18 2018, 1:56 PM

Alright. This changes many manual code to code that is dynamic, easy to understand and to maintain (for both LTR and RTL layouts).

  1. I've used QRectF to get all rects correct logically, instead of all the +1 -1 done on QRect. This can be changed back, but I really see no big performance hit if we used this few number of floats.
  2. The idea in getting the rectangle of each item is to get it logically, stack each item (eye, thumbnail, text, decoration, icons) one after the another, and just offset from that for actual drawing.
  3. The removed KisNodeView::visualRect makes drawing the editing QLineEdit on the actual item, not the whole row. This means that the visibility icon, thumbnail icon and the offset is shown in edit mode. I bet the user will ever rename a layer that he can't see it's name (besides that it can be seen from the tooltip, but who will ever rename depending on this?).

Now there is one very strange issue: When you hide/show a layer, the painting of the row isn't updated instantly. I have no idea how this broke or how to fix it. While I was working on this, it broke for RTL but not LTR, after that it broke for both.

Last thing, the commented-out code in`KisNodeDelegate::editorEvent` was an idea for changing the cursor to show the user what selection mode action will be performed (PS does this, and I thought that this code is really buried with no way for the user to know it's existence) . It didn't work quite will, and I'll remove it later on.

rempt added a subscriber: rempt.Sep 20 2018, 8:11 AM

I'm all for this thing -- please don't think we're ignoring you . I'm just really preoccupied with the fund raiser, so it'll be some time before I can look at it.

Hi! How are you doing with this patch? Should we look at it, or is it still work-in-progress?

safaalfulaij planned changes to this revision.Oct 29 2018, 6:33 PM

Hi :)
tbh, I stopped working on it once I uploaded it. I'll get back to it soon, but need to fix my Linux working machine.
I will most probably change QRectFs to QRects again, and maybe make the comments a bit better.
Then I have to figure out why rendering of the items isn't updated in the moment of showing/hiding the layer.

Update based on comments given by bond

Minor changes

Hi, @safaalfulaij!

Could you please make a screenshot and a bit of explanation of what this patch is expected to do? I tried to add RTL text to the layer name and it seem to be rendered kind of fine with and without your patch. The only problem I have found is the alignment of the text (it is left-aligned), but I'm not sure it is what you are trying to solve.

safaalfulaij added a comment.EditedDec 26 2018, 4:52 PM

Hi @dkazakov!
Well, basically (please ignore the Arabic/English text changing):
Before:

After:

What I mean here by RTL is the RTL layout that is used in the UI in case the translation's language is RTL, like Arabic or Hebrew. Qt supports RTL text very well :)
Now about the alignment, it's better to keep it as is, left-aligned if UI is LTR, right-aligned if UI is RTL.
But if we talk about the direction, then it's maybe better to have proper direction depending on the text. I mean by direction whether to keep (for example) the dot at right or at left. “layer.” vs “.LAYER” (assuming capital letters are Arabic letters).
But this is actually a problem in all Qt static text (labels, check boxes, radio buttons and list items), and will require fixing each and every list.
Maybe it's just ok if the layers panel support that right now, since it is the thing that the user will work with all the time, and rename things constantly.

dkazakov requested changes to this revision.Dec 29 2018, 11:28 AM

Hi, @safaalfulaij!

Thank you for sharing the screenshots! It looks like the problem is really serious. I would say that alignment is almost "not-a-problem" in comparison to the main part of the patch :)

I have tested the patch on a standard LTR system and it seems like it has three small regressions. If you manage to fix them, we can merge this patch into master freely :)

  1. When creating a layer with on-standard set of switches, the switches are offset by one pixel.
  2. When hovering with the mouse, the horizontal lines sometimes disappear
  3. The "collapse" arrow now almost touches the icon. Before the patch, it was centered in its slot (I compared with the vanilla version of Krita)

Here is a video of the problem:

This revision now requires changes to proceed.Dec 29 2018, 11:28 AM

Addressed comments.

dkazakov accepted this revision.Jan 8 2019, 10:43 AM

Hi, safaalfulaij!

The patch seem to cause no regressions in LTR mode, so it is safe to push it! :)

I also tried to activate Arabic locale here locally, but it didn't flip the docker for some reason. So I'll just trust your testing of the feature :)

Please don't forget to remove the debugging line before pushing!

libs/ui/KisNodeDelegate.cpp
490

Please remove this debug before comitting :)

This revision is now accepted and ready to land.Jan 8 2019, 10:43 AM
This revision was automatically updated to reflect the committed changes.