feat: new cmdline (local to editor view) #57

gikari · 2023-02-26T19:54:57Z

Summary

This creates Neovim command line inside the editor, instead of occupying Qt Creator search bar. This is an enhancement, because:

This makes it more prominent and easier to notice (the cmd lines spawns inside the split we are currently viewing).
We could write longer lines for commands.
We could now see full messages Neovim prints. If there is not enough place, we can scroll the message.

Notes

I found some strange behavior when closing splits, but it existed before my change.
When you start typing the text in the command line, but then change focus using mouse to editor, your cursor is forced to be in the command line, but the editor highlights behave strangely. I am not sure how this behavior should be handled, because in Neovim, when you click the mouse, nothing really changes: you could only select text in the splits, but command line stays active. Should we do the same thing, or should we discard the command line and bring the focus to the editor, that was clicked on?

Test Plan

Press :, / or ? and try to use some neovim command. It should work.
Try to move cursor inside command line. It should work.
Try to print message in the command line, e.g. using an incorrect command.
Try to print multiline message in the command line using :history or echo "something\n\n\n\n\nsomething".
Find other ways to break the command line.

Closes #56

sassanh · 2023-02-26T21:45:59Z

Thanks! I tried it on my machine and it looks very nice, but I need more time to review the code.

I found some strange behavior when closing splits, but it existed before my change.

Externalizing UI has improved a lot these years, if we drop neovim-qt and upgrade to the latest API we should be able to drop lots of hacks we currently have in the codebase and drop the complexity and improve the stability for a wide range of use cases. Using ext_windows and ext_multigrid for example should resolve issues with splits, etc.
I say let's wait for libneovim to be stable and then we plan to refactor our codebase.

When you start typing the text in the command line, but then change focus using mouse to editor, your cursor is forced to be in the command line, but the editor highlights behave strangely. I am not sure how this behavior should be handled, because in Neovim, when you click the mouse, nothing really changes: you could only select text in the splits, but command line stays active. Should we do the same thing, or should we discard the command line and bring the focus to the editor, that was clicked on?

I think we better stick to Neovim behavior whenever possible. Generally speaking people using this plugin probably are satisfied with Neovim user experience and are used to it and prefer to stick with it whenever possible.
For example all Vim users know clicking mouse anywhere when cmdline-mode is active won't change anything, now if we change this behavior one may get surprised to see all he has typed blindly were inserting in the buffer instead of being typed in the cmdline just cause his fingered touched his mousepad accidentally. So I think there is value in being consistent with Neovim user experience.

gikari · 2023-02-28T18:50:48Z

I think we better stick to Neovim behavior whenever possible. Generally speaking people using this plugin probably are satisfied with Neovim user experience and are used to it and prefer to stick with it whenever possible. For example all Vim users know clicking mouse anywhere when cmdline-mode is active won't change anything, now if we change this behavior one may get surprised to see all he has typed blindly were inserting in the buffer instead of being typed in the cmdline just cause his fingered touched his mousepad accidentally. So I think there is value in being consistent with Neovim user experience.

I figured out a way to block the editors interaction with the mouse during command line editing, but I am not sure if it is possible to let the user select text in the buffer. Futhermore, the user can still open a file/split using GUI, which you could not do in Neovim. Should we block this functionality as well?

gikari · 2023-02-28T19:19:46Z

Externalizing UI has improved a lot these years, if we drop neovim-qt and upgrade to the latest API we should be able to drop lots of hacks we currently have in the codebase and drop the complexity and improve the stability for a wide range of use cases. Using ext_windows and ext_multigrid for example should resolve issues with splits, etc. I say let's wait for libneovim to be stable and then we plan to refactor our codebase.

ext_multigrid

I see, that in the code, we already pass an ext_multigrid option and neovim-qt sends us a multigrid events such as win_viewport and grid_line (for some reason very rapidly, there might be a bug somewhere). Is it possible to utilize that?

If I am not mistaken, ext_windows is in a stage of pull request for neovim currently?

sassanh · 2023-02-28T22:50:54Z

I figured out a way to block the editors interaction with the mouse during command line editing, but I am not sure if it is possible to let the user select text in the buffer. Futhermore, the user can still open a file/split using GUI, which you could not do in Neovim. Should we block this functionality as well?

I don't think we should block anything (even mouse interaction) I just think it would be better to stick with Neovim user experience. If we accidentally get extra features it's good. So I think it's good that interacting with mouse doesn't discard cmdline (Neovim behavior kept, so users will feel at home), I'm also happy interacting with mouse is possible (extra features, if someone accidentally interacts with the mouse and copies some text from the buffer by mouse, good for them.)

Regarding opening a file/split using GUI, I think it's ok, we don't need to block or manipulate anything. Ideally we should just report it to Neovim and let Neovim instance decide what to do in this situation. If Neovim ignores it or if it opens the window/split while keeping the user in cmdline or if Neovim opens the window/split and discards cmdline, it's all good. If Neovim crashes, then it's bad and we will report a bug in Neovim repository. I say let the Neovim team think about such issues, as they are designing the external user interface and its behavior, if something goes wrong we fix it in Neovim, not in qnvim.

The thing is clicking on "new window"/"new split" button in an IDE/editor while editing cmdline is not something special with qnvim, it can happen in VSCode, JetBrains, Sublime or any other IDE/editor, so for the sake of consistency and avoiding duplications we better decide about it in Neovim.

Sorry for the long post, I tried to explain how I understand this issue.

sassanh · 2023-02-28T22:56:19Z

I see, that in the code, we already pass an ext_multigrid option and neovim-qt sends us a multigrid events such as win_viewport and grid_line (for some reason very rapidly, there might be a bug somewhere). Is it possible to utilize that?

If I am not mistaken, ext_windows is in a stage of pull request for neovim currently?

Sorry, I forgot we already use ext_multigrid.

My knowledge about new Neovim api is poor, so I can't brainstorm with you about it. I'm willing to learn about it once libnvim is out and stable and we can easily and directly communicate with Neovim instance with a third party library. It's beyond the scope of this pull request anyway. I think the user experience of cmdline in this pull request is already very good.

sassanh · 2023-02-28T22:57:52Z

Relevant: neovim/neovim#21693

sassanh

Sorry for the late review, I added some comments.

sassanh · 2023-04-08T15:53:35Z

src/automap.h

+ */
+template <typename K, typename V>
+requires (QObjectBasedPointer<K> || QObjectBasedPointer<V>)
+class AutoMap : public AutoMapBase {


I need to learn a bit about its mission and the rational behind implementing it.

It seems to me that this data-structure has two missions:

Act as a map.

Help with garbage collection.

If that's correct then it may be an anti-pattern regarding Single Responsibility Principle and may make the maintenance of the code hard over time.
An alternative would be to implement a list of pairs for this purpose instead of a map and keep the lookup feature for a simple Qt or std map?

Also I'm under the impression that it will delete the key if the value is deleted and also deletes the value if the key is deleted. So if the cmdline is somehow deleted it deletes the editor and the editorview, right? Is it intentional?

Also in this project we have avoided standard library and sticked with Qt data structures (like using QMap instead of std::map). What do you think about keep this pattern for the sake of consistency?

sassanh · 2023-04-08T16:26:40Z

src/qnvimcore.cpp

@@ -148,13 +137,12 @@ autocmd VimEnter * let $MYQVIMRC=substitute($MYVIMRC, 'init.vim$', 'qnvim.vim',
        mNVim->api2()->nvim_subscribe("Gui");
        mNVim->api2()->nvim_subscribe("api-buffer-updates");
    });
+
+    m_cmdLine = std::make_unique<CmdLine>(this);


Since it's a QObject, Isn't new CmdLine(this) enough to take care of garbage collection? I know using new is not preferred in C++ but last time I checked (I may not be updated) for QObjects you usually don't need to take care of garbage collection and passing this as the parent in the constructor is enough for Qt to take care of the rest.
I don't have a strong opinion about this, if you are confident this is better we can go with it, I'd be thankful if you provide me a resource so that I can learn more about it.

I'm just trying to avoid standard library in parallel with Qt, my understanding is that Qt already has whatever standard library can provide for general use cases and having both work in parallel may cause some compatibility issues.

sassanh · 2023-04-08T16:36:17Z

src/qnvimcore.cpp


-    QTimer::singleShot(100, this, [=]() { fixSize(editor); });


Don't we need to call fixSize in the initialization, previously we used to call it on initialization as well as whenever Resize event was triggered.

sassanh · 2023-04-08T16:47:24Z

src/textediteventfilter.cpp

@@ -0,0 +1,47 @@
+// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>


For the sake of correctness I think the copyright header for the newly created files that are mostly based on the old code should be probably like this:

Suggested change

// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>

// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>

// SPDX-FileCopyrightText: 2018-2019 Sassan Haradji <[email protected]>

sassanh · 2023-04-19T22:33:11Z

Sorry, this PR got closed automatically when I merged qtc-6 to master, kindly reopen it when you got the time.

gikari added 4 commits February 26, 2023 20:34

feat: new cmdline (local to editor view)

9da023e

chore: add licence info to new files

c661c9f

fix: use correct casing for includes

cfadb48

fix: add unordered_map include

9dd968a

sassanh reviewed Apr 8, 2023

View reviewed changes

sassanh deleted the branch sassanh:qtc-6 April 19, 2023 22:31

sassanh closed this Apr 19, 2023

sassanh linked an issue Apr 19, 2023 that may be closed by this pull request

Better command line #56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new cmdline (local to editor view) #57

feat: new cmdline (local to editor view) #57

gikari commented Feb 26, 2023

sassanh commented Feb 26, 2023

gikari commented Feb 28, 2023

gikari commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh left a comment

sassanh Apr 8, 2023 •

edited

Loading

sassanh Apr 8, 2023

sassanh Apr 8, 2023

sassanh Apr 8, 2023

sassanh Apr 8, 2023

sassanh commented Apr 19, 2023

		@@ -0,0 +1,47 @@
		// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>

	// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>
	// SPDX-FileCopyrightText: 2023 Mikhail Zolotukhin <[email protected]>
	// SPDX-FileCopyrightText: 2018-2019 Sassan Haradji <[email protected]>

feat: new cmdline (local to editor view) #57

feat: new cmdline (local to editor view) #57

Conversation

gikari commented Feb 26, 2023

Summary

Notes

Test Plan

sassanh commented Feb 26, 2023

gikari commented Feb 28, 2023

gikari commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh commented Feb 28, 2023

sassanh left a comment

Choose a reason for hiding this comment

sassanh Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

sassanh Apr 8, 2023

Choose a reason for hiding this comment

sassanh Apr 8, 2023

Choose a reason for hiding this comment

sassanh Apr 8, 2023

Choose a reason for hiding this comment

sassanh Apr 8, 2023

Choose a reason for hiding this comment

sassanh commented Apr 19, 2023

sassanh Apr 8, 2023 •

edited

Loading