Add :vsplit and :split support in kdevelop
ClosedPublic

Authored by head7 on Jan 22 2017, 10:32 PM.

Details

Summary

Using :vsplit and :split to split the view as in vim is something that could be nice.

Test Plan

Run :vsplit the screen should get split into two new screen

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
head7 updated this revision to Diff 10443.Jan 22 2017, 10:32 PM
head7 retitled this revision from to Add :vsplit and :split support in kdevelop.
head7 updated this object.
head7 edited the test plan for this revision. (Show Details)
head7 added a reviewer: brauch.
head7 set the repository for this revision to R33 KDevPlatform.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 22 2017, 10:32 PM
brauch edited edge metadata.Jan 22 2017, 10:58 PM

One question below, otherwise very nice change, and very compact :)

shell/ktexteditorpluginintegration.cpp
215

Why is this required now?

mwolff added a subscriber: mwolff.Jan 23 2017, 7:52 AM

I also wonder about the re-parenting, otherwise lgtm except for the minor style issue

shell/ktexteditorpluginintegration.cpp
341

style: put { on its own line

head7 added inline comments.Jan 23 2017, 6:08 PM
shell/ktexteditorpluginintegration.cpp
215

In ktexteditor:src/utils/mainwindow.cpp splitView() function we have

QMetaObject::invokeMethod(parent()
                          , "splitView"
                          , Qt::DirectConnection
                          , Q_ARG(Qt::Orientation, orientation));

and this function is called from AppCommands::exec (ktexteditor:src/vimode/appcommand.cpp) on the pointer mainWin:

mainWin->splitView(Qt::Horizontal);

which is view->mainWindow() (the same kteView->mainWindow() above), so in my understanding we are then calling invokemethod on kteView->mainWindow()->parent() but this pointer was a nullptr, so I set it to the "correct" mainWindow object (this) when creating the wrapped view, but it's maybee not the best way to proceed ?

head7 updated this revision to Diff 10467.Jan 23 2017, 7:46 PM
head7 edited edge metadata.
head7 removed R33 KDevPlatform as the repository for this revision.

move { to newline

That sounds... odd - the rest of the MainWindow API we overload already works, or doesn't it? I.e. what we have in ktexteditorpluginintegration.h's MainWindow only ever gets called when it is the parent() object of the MainWindow in ktexteditor.

If that is not the case, then something is really wrong. We have a test case for this in shell/tests/test_ktexteditorpluginintegration.cpp - is that failing for you?

head7 marked an inline comment as done.Jan 30 2017, 6:55 PM
In D4250#79740, @mwolff wrote:

That sounds... odd - the rest of the MainWindow API we overload already works, or doesn't it? I.e. what we have in ktexteditorpluginintegration.h's MainWindow only ever gets called when it is the parent() object of the MainWindow in ktexteditor.

If that is not the case, then something is really wrong. We have a test case for this in shell/tests/test_ktexteditorpluginintegration.cpp - is that failing for you?

No the test succeed on my computer:

  • Start testing of TestKTextEditorPluginIntegration *****

Config: Using QtTest library 5.7.1, Qt 5.7.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 6.2.0 20161027)
QWARN : TestKTextEditorPluginIntegration::initTestCase() cannot find .rc file "test_ktexteditorpluginintegrationui.rc" for component "test_ktexteditorpluginintegration"
PASS : TestKTextEditorPluginIntegration::initTestCase()
PASS : TestKTextEditorPluginIntegration::testApplication()
PASS : TestKTextEditorPluginIntegration::testMainWindow()
PASS : TestKTextEditorPluginIntegration::testPlugin()
PASS : TestKTextEditorPluginIntegration::testPluginUnload()
PASS : TestKTextEditorPluginIntegration::cleanupTestCase()
Totals: 6 passed, 0 failed, 0 skipped, 0 blacklisted, 421ms

  • Finished testing of TestKTextEditorPluginIntegration *****

But there is no tests checking toKteView parent ?

head7 updated this revision to Diff 10738.Jan 30 2017, 10:01 PM

Thanks to commit 00de613bcb0fb0cff587e0b8ce090b915bc6f717 no more need to setParent here.

mwolff requested changes to this revision.Jan 30 2017, 10:27 PM
mwolff added a reviewer: mwolff.
mwolff added inline comments.
shell/ktexteditorpluginintegration.cpp
355

use public API, i.e. add a void split(Qt::Orientation orientation); to MainWindow and call d->split internally. so here it's just

m_mainWindow->split(orientation);
shell/mainwindow.h
51

this should be removed, instead add a

void split(Qt::Orientation orientation);

below and implement it as

void MainWindow::split(Qt::Orientation orientation)
{
    d->split(orientation);
}
This revision now requires changes to proceed.Jan 30 2017, 10:27 PM
head7 updated this revision to Diff 10743.EditedJan 30 2017, 10:43 PM
head7 edited edge metadata.

use public API

mwolff accepted this revision.Jan 30 2017, 10:53 PM
mwolff edited edge metadata.

excellent, do you have commit rights? if so, please land this in master

cheers

This revision is now accepted and ready to land.Jan 30 2017, 10:53 PM
head7 added a comment.Jan 31 2017, 8:05 AM

Just a last question, with this commit when we do a :vsplit it splits the view "horizontally" (one buffer above the other") and :hsplit split the view "vertically" (one buffer on the left of the other).

It's the same behavior as in kate, but it's the opposite as in the original vi/vim.

What do you think ?
1- Should we invert the behavior only in KdevPlatform ?
2- Should I propose a modification to the KtextEditor team, so vsplit and hsplit really mimick vi/vim ?
3- Should we keep this behavior, and hope the vim user get to this new habit ?

In my point of view the 2 solution is the best but it could break the kate user habit if they already use this feature.

that's an odd naming choice in vim then!

in my book:

  • horizontal means left/right
  • vertical means top/bottom

so from what you describe, kate does what I'd expect it to do, whereas vim does the opposite. That said, this feature is there for vim people, so having it do what vim does sounds reasonable. You will have to change this feature in ktexteditor though, please contact the corresponding people via e.g. kwrite-devel@kde.org and talk to them about this.

In D4250#81741, @mwolff wrote:

that's an odd naming choice in vim then!

in my book:

  • horizontal means left/right
  • vertical means top/bottom

    so from what you describe, kate does what I'd expect it to do, whereas vim does the opposite. That said, this feature is there for vim people, so having it do what vim does sounds reasonable. You will have to change this feature in ktexteditor though, please contact the corresponding people via e.g. kwrite-devel@kde.org and talk to them about this.

Well, wait. Horizontal in kate is top / bottom, and Vertical is Left - Right. (if you look at the icons on Edit / Split Views. I'd vote for a bit of consistency here. a Vertical Split usually means "Create Columns" and a Horizontal Split usually means "Create Rows", this is also the behavior we get by QVBoxLayout and QHBoxLayout.

In D4250#81741, @mwolff wrote:
  • horizontal means left/right
  • vertical means top/bottom

If the view is split vertically, top to bottom, the resulting areas are left/right.

Well, I don't care - as I said - make this behave like vim, but fix it in ktexteditor, not kdevelop.

@head7: do you have commit rights? if so, please commit this patch here to master, otherwise I'll do it for you eventually

head7 added a comment.EditedJan 31 2017, 4:29 PM

I don't have commit rights. So if you can push that for me it would be nice.

Thanks.

mwolff set the repository for this revision to R33 KDevPlatform.
mwolff added a project: KDevelop.

I will need your full name and your email address to use in the git commit message.

Sorry, but phabricator does not give me this information :-/

head7 added a comment.Feb 2 2017, 4:41 PM
In D4250#82448, @mwolff wrote:

I will need your full name and your email address to use in the git commit message.

Name: Laurent George
mail: laurent.f.george at gmail.com

This revision was automatically updated to reflect the committed changes.