[Differential] [Updated] D4538: [KTextEditor] consistent conversion from/to cursor to/from coordinates

[John Salatas]

jsalatas updated the summary for this revision.

REPOSITORY
  R39 KTextEditor

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

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

To: jsalatas, #frameworks, #plasma
Cc: plasma-devel, kwrite-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

[Differential] [Request, 6 lines] D4538: [KTextEditor] consistent conversion from/to cursor to/from coordinates

[John Salatas]

jsalatas created this revision.
jsalatas added reviewers: Frameworks, Plasma.
jsalatas set the repository for this revision to R39 KTextEditor.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: kwrite-devel, plasma-devel.

REVISION SUMMARY
  This patch addresses 3 issues
  
  1. Currently if you try to convert the cursor to coordinates and then convert 
these coordinates back to a cursor again, it produces an invalid cursor (-1, 
-1) when the original cursor is located at the end of line
  
  2. Converting back to cursor coordinates that have been produced by 
`cursorToCoordinate(view->cursorPosition())` and 
`coordinatesToCursor(view->cursorPositionCoordinates())` doesn't always produce 
the same results as the `includeBorder` option doesn't seem to be used 
consistently.
  
  3. Although I cannot find a test case for this, it doen't seem to make any 
sense the line `scrollPos(max, max.column(), calledExternally);` in 
`KateViewInternal::makeVisible` function.   Was it supposed to be that way (ie 
force when the last line is not empty and not force when it is), or is it just 
a typo? :\  I would appreciate any reasoning about this!

TEST PLAN
  1. Create a new app with a single `KTextEditor::View` widget (similar to 
kwrite).
  
  2. connect the view's `cursorPositionChanged` signal with a slot in the 
window which contains the following code
  
qDebug() << " original cursor" << view->cursorPosition();
QPoint p1 = view->cursorToCoordinate(view->cursorPosition());
KTextEditor::Cursor c1 = view->coordinatesToCursor(p1);
qDebug() << "converted cursor" << c1;
qDebug() << "converted cursor 2" << 
view->coordinatesToCursor(view->cursorPositionCoordinates());
  
  
  
  3. Run the application and start typing (I typed 52 characters in the first 
line) and then pressed the  key sometimes)
  
  4. Press Back Arrow key until you are in the beginning of the document.
  
  In steps 3 and 4 the three cursors are the same (this wasn't the case before).
  
  Before you get something like the following
  
original cursor (0, 1)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 2)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 3)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 4)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 5)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 6)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 7)
converted cursor (-1, -1)
..
 original cursor (0, 48)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 49)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 50)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 51)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 52)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (1, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (2, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (3, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
.
 original cursor (0, 52)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 51)
converted cursor (0, 51)
converted cursor 2 (-1, -1)
 original cursor (0, 50)
converted cursor (0, 50)
converted cursor 2 (-1, -1)
 original cursor (0, 49)
converted cursor (0, 49)
converted cursor 2 (-1, -1)
 original cursor (0, 48)
converted cursor (0, 48)
converted cursor 2 (-1, -1)
 original cursor (0, 47)
converted cursor (0, 47)
converted cursor 2 (0, 51)
 original cursor (0, 46)
converted cursor (0, 46)
converted cursor 2 (0, 50)
 original cursor (0, 45)
converted cursor (0, 45)
converted cursor 2 (0, 49)
 original cursor (0, 44)
converted cursor (0, 44)
converted cursor 2 (0, 48)
 original cursor (0, 43)
converted cursor (0, 43)
converted cursor 2 (0, 47)
 original cursor (0, 42)
converted cursor (0, 42)
converted cursor 2 (0, 46)
 original cursor (0, 41)
converted cursor (0, 41)
converted cursor 2 (0, 45)
.
 original cursor (0, 6)
converted cursor (0, 6)
converted cursor 2 (0, 10)
 original cursor (0, 5)
converted cursor (0, 5)
converted cursor 2 (0, 9)
 original cursor (0, 4)
converted cursor (0, 4)
converted cursor 2 (0, 8)
 original cursor (0, 3)
converted cursor (0, 3)
converted cursor 2 (0, 7)
 original cursor (0, 2)
converted cursor (0, 2)
converted cursor 2 (0, 6)
 original cursor (0, 1)
converted 

[Differential] [Request, 109 lines] D4537: EditorConfig support

[Grzegorz Szymaszek]

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

REVISION SUMMARY
  This diff enables support for reading configurations from `.editorconfig` 
files, as an alternative to already supported `.kateconfig`. EditorConfig 
 is supported in many other editors and IDEs. In case 
of KTextEditor it’s done using editorconfig-core-c 
 library that I’ve added 
to `CMakeLists.txt` and `katedocument.h`. A disadvantage of my approach is that 
this library became a KTextEditor’s dependency, maybe it should be made 
optional?
  Fixes bug 330843 .

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/CMakeLists.txt
  src/document/katedocument.cpp
  src/document/katedocument.h

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

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

[Differential] [Accepted] D4522: export implicitheight from background

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  mart/qqc2style

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

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

To: mart, davidedmundson
Cc: #frameworks

[Differential] [Request, 17 lines] D4522: export implicitheight from background

[Marco Martin]

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

REVISION SUMMARY
  all indicators are not meant to be exported

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  mart/qqc2style

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/Button.qml
  src/declarativeimports/plasmacomponents3/CheckIndicator.qml
  src/declarativeimports/plasmacomponents3/ToolButton.qml
  src/declarativeimports/plasmacomponents3/qmldir

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

To: mart
Cc: #frameworks

Re: kwidgetsaddons on OS X: menus become menu items

[David Faure]

On mardi 7 février 2017 10:21:33 CET René J.V. Bertin wrote:
> On Tuesday February 7 2017 01:15:23 Marko Käning wrote:
> 
> Hi,
> 
> >BTW, has this problem disappeared in the meantime?
> 
> Not really. The Qt error messages are no longer printed, but the underlying
> cause is still there. QActions that are added to a Mac native menu are
> reparented and cannot also be used elsewhere. I know David Faure has picked
> up on that a while back, I don't know if he has managed to get any clarity
> on the why and how of this regression (w.r.t. Qt4).

No, no idea, but this certainly can't be expected, the whole point of the 
QAction concept is to use the *same* QAction in menus, toolbars, context 
menus, and for shortcuts. So it's most definitely a Qt bug.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

[Differential] [Commented On] D4508: [WIP] Plasma controls based on QtQuickControls2

[David Edmundson]

davidedmundson added a comment.


  I want to do another full review next week when I'm home; but I'd be happy 
with doing that after we merge it, as it'll only be small things, and nothing 
which will affect API.
  
  In general it all looks very good, I like that you split out ButtonShadow and 
all the rest seems neat. Good stuff.

INLINE COMMENTS

> Button.qml:28
> +
> +implicitWidth: Math.max(background ? background.implicitWidth : 0,
> +contentItem.implicitWidth + leftPadding + 
> rightPadding)

I don't get why we check background here and in other places. It would only be 
null if a user subclassed this and then overwrote the property to undefined, at 
which point they deserve errors.

> Button.qml:51
> +//retrocompatibility with old controls
> +implicitWidth: units.gridUnit * 6
> +Private.ButtonShadow {

we do someting differently for widths and heights which isn't ideal, can we 
move the units.gridUnit * 1.6 into here as an implicitHeight?

> CheckIndicator.qml:32
>  opacity: control.enabled ? 1 : 0.6
>  property int checkState: control.checkState
>  

why?

> GroupBox.qml:34
>  
>  padding: 6
>  topPadding: padding + (label && label.implicitWidth > 0 ? 
> label.implicitHeight + spacing : 0)

booo!

> Label.qml:27
>  
>  height: Math.round(Math.max(paintedHeight, units.gridUnit * 1.6))
>  verticalAlignment: lineCount > 1 ? Text.AlignTop : Text.AlignVCenter

Now we have an API break, lets fix this.

As soon as you put this in a layout it won't do anything, so it's quite 
inconsistent.
If anything it should be an implicitHeight, or just set the lineHeight?

But I'd rather we didn't have it at all, we have to work round it in lots of 
places.

If we want a padded Label, we can make a new subclass in the import with all 
the other headings.

> Label.qml:31
>  activeFocusOnTab: false
>  renderType: Text.NativeRendering
>  

Bhushan did a patch for P-F that changed this on the current label if we're on 
mobile, is that still possible?

> TextField.qml:60
>  
>  background: Item {
>  Private.TextFieldFocus {

can we kill an item here?

background: FrameSvgItem {

  TextFiledFocus {
  anchors.fill:parent
   }

}

> qmldir:39
> +ToolButton 3.0 ToolButton.qml
> +ToolTip 3.0 ToolTip.qml

maybe we don't want:

ToolTip, Popup, Dialog, DialogButtonBox here and only in the style?

Also which is the "correct" ItemDelegate we have one of them in PlasmaExtras

REPOSITORY
  R242 Plasma Framework (Library)

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

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

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