Using :vsplit and :split to split the view as in vim is something that could be nice.
Details
- Reviewers
mwolff brauch - Group Reviewers
KDevelop - Commits
- R32:bc7a8cac87a3: Add :vsplit and :split support in kdevelop
R33:bc7a8cac87a3: Add :vsplit and :split support in kdevelop
Run :vsplit the screen should get split into two new screen
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
One question below, otherwise very nice change, and very compact :)
shell/ktexteditorpluginintegration.cpp | ||
---|---|---|
215 | Why is this required now? |
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 |
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 ? |
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 ?
Thanks to commit 00de613bcb0fb0cff587e0b8ce090b915bc6f717 no more need to setParent here.
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); } |
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.
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.
- 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
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 :-/