[Differential] [Updated] D4229: Overhaul group popup dialog

[Eike Hein]

hein marked 7 inline comments as done.
hein added a comment.


  Will update once I have a Qt again.

INLINE COMMENTS

> broulik wrote in ContextMenu.qml:537
> Unrelated

Will do.

> broulik wrote in GroupDialog.qml:63
> Remove

Will do.

> broulik wrote in GroupDialog.qml:113-114
> Why this change?

I was doing heavy refactoring and changing the nesting and trying different things a few times, happened along the way. I'll clean it out of the diff.

> broulik wrote in GroupDialog.qml:119
> FTR: Qt 5.9 will add a "positioningComplete" signal :)

Nifty, about time. That also means we can remove some fugly code in main.qml for the layouting and delegate geo publishs.

> broulik wrote in GroupDialog.qml:156
> Can you perhaps have the task do that automatically since fwict you always do forceActiveFocus and then ensureItemVisible:
> 
>   onActiveFocusChanged: {
>       if (activeFocus) {
>           ensureItemVisible(this);
>       }
>   }

I see the appeal, but I'd rather not do it implicitly and calling GroupDialog API ... I'm rather unhappy with how much coupling there is between Task and  GroupDialog already (writing a delegate that relies on API outside of it is always pretty ugly). I'll think about whether I can find some other nice solution.

> broulik wrote in GroupDialog.qml:253
> ?

Doing it declaratively creates a binding loop in this case.

> broulik wrote in GroupDialog.qml:264
> Why not assign it directly above?
> 
>   property TextMetrics textMetrics: TextMetrics {}

I think this wasn't possible in older Qt Quicks? Nice that it's possible now, will do.

REPOSITORY
  R119 Plasma Desktop

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

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

To: hein, #plasma
Cc: broulik, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170123/2351483e/attachment.html>