Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vimode: Added folding support #1327

Closed
wants to merge 0 commits into from

Conversation

scresto09
Copy link
Contributor

Supported commands are:
za / zo / zc toggle / open / close fold on one level of folding
zA / zO / zC toggle / open / close fold on all folding levels
zR / zM open / close all folds

@techee
Copy link
Member

techee commented Apr 25, 2024

Looks good in general, but I think folding should work when the cursor is anywhere in the block, not just on the fold point (vim seems to work this way). I played with it a little and came up with something like this:

static gint prepare_fold(CmdParams *p)
{
	/* foldparent of the next line */
	gint line = SSM(p->sci, SCI_GETFOLDPARENT, p->line + 1, 0);

	if (p->line == line)
		;  /* we are already on the fold point line */
	else
	{
		/* foldparent of the current line */
		line = SSM(p->sci, SCI_GETFOLDPARENT, p->line, 0);
	}

	if (line != -1)
	{
		/* move the cursor on the visible line before the fold */
		gint pos = SSM(p->sci, SCI_POSITIONFROMLINE, line, 0);
		SET_POS_NOX(p->sci, pos, TRUE);
	}

	return line;
}


void cmd_toggle_fold(CmdContext *c, CmdParams *p)
{
	gint line = prepare_fold(p);
	if (line != -1)
		SSM(p->sci, SCI_FOLDLINE, (uptr_t) line, SC_FOLDACTION_TOGGLE);
}

If the code looks OK to you and works as expected, could you update all the folding functions similarly to cmd_toggle_fold() above?

In addition, these aren't exactly "edit" commands and should be placed in a separate file. Could you create fold.c/h, add them to Makefile.am and move all these commands there?

@techee
Copy link
Member

techee commented May 21, 2024

If you are planning to continue working on this PR, there's one catch. Because of

33984ab

you have to first move the cursor on the visible line before performing a fold - otherwise folding won't work because the above code will expand the fold (I experienced this problem myself when testing the code above and it took me a while to realize what was going on).

@scresto09
Copy link
Contributor Author

OK, I tried to make some changes to improve fold support and react more like vim.
With za/zo/zc we fold the current block as before.
With zA we change the fold depending on the state of the parent, if there is one that is contracted we expand it otherwise we contract it.
I was unable to reproduce the issue you are talking about with commit 33984ab, let me know if the issue persists.
And another thing, sorry but I'm not sure I did the right things with git/merge, let me know if I need to fix anything on this.
Thanks.

Copy link
Member

@techee techee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good functionality-wise, I have just suggested to rename some things to make them clearer (at least to me).

I was unable to reproduce the issue you are talking about with commit 33984ab, let me know if the issue persists.

When you call prepare_fold() the cursor is moved above it so you don't see it. But you can reproduce it with cmd_close_fold_all() which currently doesn't call prepare_fold() (but it should) and it doesn't collapse the fold where the cursor is placed - see the comment below.

It would be best if you rebased this PR on top of current geany-plugins master as there's a merge conflict in the Makefile.

/* fold command depend on state of parents */
#define FOLD_PARENT 1
/* fold command depend on state of contracted parent if it exists */
#define FOLD_CONTRACTED 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was super-confused by the naming. I's suggest the following:

  • FOLD_NONE -> GOTO_NEAREST_PARENT
  • FOLD_PARENT -> GOTO_TOPMOST_PARENT
  • FOLD_CONTRACTED -> GOTO_CONTRACTED_PARENT

plus the corresponding comment changes.

/* fold command depend on state of contracted parent if it exists */
#define FOLD_CONTRACTED 2

static gint prepare_fold(CmdParams *p, gint filter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to goto_above_fold() and the filter parameter to type.



void cmd_close_fold_all(CmdContext *c, CmdParams *p)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need goto_above_fold(p, GOTO_TOPMOST_PARENT); otherwise the fold where the cursor is won't get contracted because of what I mentioned before.

}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra empty line.

@techee
Copy link
Member

techee commented May 22, 2024

One more thing, the README file contains all the supported vi commands - would you copy the added commands from index.txt in the vimode directory into README?

@techee
Copy link
Member

techee commented May 23, 2024

@scresto09 You seem to have pushed an unmodified branch and the pull request closed as a result. There are no commits in this pull request now.

@scresto09
Copy link
Contributor Author

Sorry, I made a mistake and this closed this pull request...

I just created a new pull request #1350 with all these changes.. sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants