Implement the next/previous editor commands and keybindings by leavengood · Pull Request #1447 · onivim/oni2

leavengood · 2020-03-08T19:35:23Z

This wraps around like VSCode.

Fixes #778 and part of #1423.

CLAassistant · 2020-03-08T19:36:32Z

All committers have signed the CLA.

This wraps around like VSCode. Fixes onivim#778 and part of onivim#1423.

leavengood · 2020-03-08T19:53:22Z

Hmmm, I just did some testing in VSCode and I think this works a bit different than I thought. If you have just one editor group, it moves between the tabs in that group (which is what I implemented), if you have multiple editor groups it moves between them when you reach each end (with full wrap around.) That is probably why the command has workspace as a prefix.

This is probably still a good first step, but I may be able to implement the full version too at a later date, if we want it.

glennsl

Nice work @leavengood! I think the approach overall looks good, but have commented on a few nits below. And most of it seems to be caused by our own code being a bad example of how to do things!

src/Model/Actions.re

src/Model/EditorGroup.re

glennsl · 2020-03-08T21:02:59Z

You should also run esy format before pushing to make sure the CI is happy.

glennsl · 2020-03-08T21:03:46Z

This is probably still a good first step, but I may be able to implement the full version too at a later date, if we want it.

Yes, I agree on both counts!

Resolve the TODO for _getIndexOfElement by making it return option(int). List.find_opt is not actually the right choice to get an index. Also ran `esy format`.

leavengood · 2020-03-09T04:04:15Z

I believe I have addressed most of the comments. I am still not very happy with how awkward the setActiveEditorTo function is in EditorGroup, but it works. The need for wrap around makes other ways of iterating through the list less workable.

glennsl

Looks good! I'd consider this mergable if you're happy, but still have a few suggestions below you might consider:

glennsl · 2020-03-09T10:26:08Z

src/Model/EditorGroup.re

+        switch (_getIndexOfElement(elem, tl)) {
+        | None => None
+        | Some(i) => Some(i + 1)
+        }


You can use Option.map for this:

Suggested change

switch (_getIndexOfElement(elem, tl)) {

| None => None

| Some(i) => Some(i + 1)

}

_getIndexOfElement(elem, tl) |> Option.map(succ)

I figured there had to be something for that like in Rust but when looking at the Reason docs I did not see much. I need to check the OCaml docs more I suppose. Good tip with using succ too.

glennsl · 2020-03-09T10:36:43Z

src/Model/EditorGroup.re

+        let newIndex =
+          if (newIndex < 0) {
+            // Wrapping negative, go to end
+            count - 1;
+          } else if (newIndex >= count) {
+            0;
+            // If this is past the end, go to zero
+          } else {
+            newIndex;
+          };


There are some convenience functions in Core.Utility.IndexEx for this:

oni2/src/Core/Utility/IndexEx.re

Lines 1 to 27 in 7493356

let prevRollOver = (~first=0, ~last, current) =>

if (first >= last) {

first;

} else if (current <= first) {

last;

} else {

current - 1;

};

let prevRollOverOpt = (~first=0, ~last) =>

fun

| Some(index) => Some(prevRollOver(index, ~first, ~last))

| None => Some(last);

let nextRollOver = (~first=0, ~last, current) =>

if (first >= last) {

first;

} else if (current >= last) {

first;

} else {

current + 1;

};

let nextRollOverOpt = (~first=0, ~last) =>

fun

| Some(index) => Some(nextRollOver(index, ~first, ~last))

| None => Some(first);

Yep, that is exactly what I need. I'll give it a try when I get a chance.

leavengood · 2020-03-09T16:13:16Z

By the way, I just want to say I appreciate all the time and guidance on the reviews. I know it can be a lot of work, as I am frequently on the other side.

glennsl · 2020-03-09T19:46:16Z

Happy to do it! I enjoy nitpicking 🙃

bryphe · 2020-03-10T00:39:10Z

Such a great addition! Thank you for all the help with this @leavengood and @glennsl 🎉

Can't wait to start using this.

bryphe · 2020-03-10T00:39:27Z

Looking at the CI - looks like an intermittent test failure, not related to the changes - kicking off a new run.

bryphe · 2020-03-10T00:39:32Z

/azp run

leavengood · 2020-03-10T00:43:39Z

Thanks @bryphe! This is a really great project, I am amazed how nice it is already and I am glad to help how I can.

If CI passes I think we can merge this as is, and I can do a bit more clean-up when I work on the setting editor by number commands. I also would like to add a test for EditorGroup then as well.

leavengood · 2020-03-10T03:41:57Z

I think this CI failure is not from my code again ☹️

Is there any way to speed up the CI? Taking 30-60 minutes seems pretty long to me. I don't know if this is something people can contribute to fix, of course it is probably very complicated with esy and dune, Azure, multiple operating systems, etc.

leavengood · 2020-03-10T03:42:34Z

/azp run

azure-pipelines · 2020-03-10T03:42:39Z

Commenter does not have sufficient privileges for PR 1447 in repo onivim/oni2

glennsl · 2020-03-10T06:00:10Z

/azp run

glennsl · 2020-03-10T06:08:24Z

Is there any way to speed up the CI?

I think @bryphe is pretty on top of this, constantly tweaking the Ci process, but it seems tricky. I think caching was brought back a few days ago, but it's often failed for us for various reasons. And the Windows CI is particularly slow and finicky, and often just hangs.

So I think it'd be hard to contribute to this, especially without a good overview of what's been tried before and what might seem to work in the short term but has proven to not work over time.

leavengood · 2020-03-10T19:34:43Z

As noted now that CI passed I think we can merge this as is and I can do some of the adjustments Glenn recommended in future work.

bryphe · 2020-03-10T20:14:50Z

I think @bryphe is pretty on top of this, constantly tweaking the Ci process, but it seems tricky. I think caching was brought back a few days ago, but it's often failed for us for various reasons.

Yeah, the CI is always a challenge. When the caching works - it really speeds up the builds! But it tends to be fragile - all kinds of things can break it (ie, recently Azure Pipelines changed the default paths, which broke our cache directories). Some previous battles with the build cache: https://github.com/onivim/oni2/pulls?q=is%3Apr+cache+is%3Aclosed

I just noticed we weren't restoring the cache builds for OSX - so I opened a PR for that: #1458

One thing we are missing is we don't have a cache strategy at all for our one environment that builds on Docker (the CentOS build) - it'd be nice to have that cached, because it's the bottleneck if the caches are working everywhere else.

It can be contributed to for sure - the files that control it are the azure-pipelines.yml file and the [.ci] folder (https://github.com/onivim/oni2/tree/master/.ci) but it can be a grind! Tends to be very time consuming to test and iterate on fixes & changes.

As noted now that CI passed I think we can merge this as is and I can do some of the adjustments Glenn recommended in future work.

Sounds great! Merging now 🎉

Thanks @leavengood for all the work on this! It's an excellent contribution.

Implement the next/previous editor commands and keybindings

95dc0f0

This wraps around like VSCode. Fixes onivim#778 and part of onivim#1423.

leavengood force-pushed the feature/workspace/editor-switching branch from 6a9ac8e to 95dc0f0 Compare March 8, 2020 19:40

glennsl reviewed Mar 8, 2020

View reviewed changes

leavengood added 2 commits March 8, 2020 23:40

Remove unneeded new actions

dee0559

Adjust how the next/previous editor code works in EditorGroup

18a29c7

Resolve the TODO for _getIndexOfElement by making it return option(int). List.find_opt is not actually the right choice to get an index. Also ran `esy format`.

glennsl approved these changes Mar 9, 2020

View reviewed changes

bryphe merged commit 39477a2 into onivim:master Mar 10, 2020

glennsl mentioned this pull request Mar 10, 2020

Keybinding Suggestions and Contribution Opportunities #1423

Open

	let prevRollOver = (~first=0, ~last, current) =>
	if (first >= last) {
	first;
	} else if (current <= first) {
	last;
	} else {
	current - 1;
	};

	let prevRollOverOpt = (~first=0, ~last) =>
	fun
	\| Some(index) => Some(prevRollOver(index, ~first, ~last))
	\| None => Some(last);

	let nextRollOver = (~first=0, ~last, current) =>
	if (first >= last) {
	first;
	} else if (current >= last) {
	first;
	} else {
	current + 1;
	};

	let nextRollOverOpt = (~first=0, ~last) =>
	fun
	\| Some(index) => Some(nextRollOver(index, ~first, ~last))
	\| None => Some(first);

Comments

Conversation

leavengood commented Mar 8, 2020

Uh oh!

CLAassistant commented Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leavengood commented Mar 8, 2020

Uh oh!

glennsl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glennsl commented Mar 8, 2020

Uh oh!

glennsl commented Mar 8, 2020

Uh oh!

leavengood commented Mar 9, 2020

Uh oh!

glennsl left a comment

Choose a reason for hiding this comment

Uh oh!

glennsl Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

leavengood Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

glennsl Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

leavengood Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

leavengood commented Mar 9, 2020

Uh oh!

glennsl commented Mar 9, 2020

Uh oh!

bryphe commented Mar 10, 2020

Uh oh!

bryphe commented Mar 10, 2020

Uh oh!

bryphe commented Mar 10, 2020

Uh oh!

leavengood commented Mar 10, 2020

Uh oh!

leavengood commented Mar 10, 2020

Uh oh!

leavengood commented Mar 10, 2020

Uh oh!

azure-pipelines bot commented Mar 10, 2020

Uh oh!

glennsl commented Mar 10, 2020

Uh oh!

glennsl commented Mar 10, 2020

Uh oh!

leavengood commented Mar 10, 2020

Uh oh!

bryphe commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Mar 8, 2020 •

edited

Loading