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

Fix RTL inverse horizontal arrow key movement #10334

Closed
wants to merge 1 commit into from

Conversation

Ashfaaq18
Copy link
Contributor

Fix #8553, Fix #7678 and Fix #9730

Before the fix:

rtlArrowKeyFixbefore

After the fix:

rtlArrowKeyFixafter

@mere-human
Copy link
Contributor

Since it seems like a problem in Scintilla code you better submit your fix directly to Scintilla project.
So we won't have problems with merging.

@Yaron10
Copy link

Yaron10 commented Aug 5, 2021

@Ashfaaq18,

I've been using SCI_ASSIGNCMDKEY in case IDM_EDIT_RTL (NppCommands.cpp).
Your solution seems better. 👍


I'd be glad if you could have a look at #8520 and #8730.

Thank you.

@Ashfaaq18
Copy link
Contributor Author

@mere-human, referring to this RTL issue ticket in the scintilla project https://sourceforge.net/p/scintilla/bugs/1590/, it seems like they've already fixed and closed it. Nevertheless, Ill still try to replicate the problem in that project. If I do encounter it, ill apply this fix and open a PR there.

And @Yaron10, yeah sure. Ill take a quick look at it.

@chcg chcg added the scintilla dependent Can't be considered for N++ implementation unless/until Scintilla changes label Aug 7, 2021
@donho
Copy link
Member

donho commented Aug 8, 2021

@Ashfaaq18

it seems like they've already fixed and closed it.

It has been fixed in v4.1.0, according Scintilla author. However, the Scintilla used by Notepad++ is v4.4.6.
I'm not sure if it's a regression in Scintilla, or Scintilla is misused in Notepad++ for causing this bug. If it's the 2nd case, it would be better to fix it in the layer of Notepad++.

Nevertheless, Ill still try to replicate the problem in that project. If I do encounter it, ill apply this fix and open a PR there.

Yes, it sounds good - thank you. I'm gonna keep this PR opened until the situation of this issue is more clear.

@donho donho self-assigned this Aug 8, 2021
@donho donho added the suspended label Aug 8, 2021
@donho donho removed the suspended label Dec 28, 2021
@donho
Copy link
Member

donho commented Dec 28, 2021

@Yaron10

I've been using SCI_ASSIGNCMDKEY in case IDM_EDIT_RTL (NppCommands.cpp).

Do you build your Notepad++ or you use the script for it?
Could you pass your list of key for binding so I may find a way to do it in IDM_EDIT_RTL ?

Your solution seems better. 👍

If Scintilla were maintained by Notepad++, it would be a better solution.
But then there are also DELETE key and DELETEBACK.

@Yaron10
Copy link

Yaron10 commented Dec 28, 2021

@donho,

I'm using the following code in NppCommands.cpp.

case IDM_EDIT_RTL:
case IDM_EDIT_LTR:
{
	bool toRTL = id == IDM_EDIT_RTL, isRTL = _pEditView->isTextDirectionRTL();
	if ((toRTL && isRTL) || (!toRTL && !isRTL))
	{
		::MessageBeep(MB_OK);
		break;
	}

	_pEditView->changeTextDirection(toRTL);
	_pNonEditView->changeTextDirection(toRTL);

	// Wrap then !wrap to fix problem of mirror characters
	bool isWraped = _pEditView->isWrap();
	_pEditView->wrap(!isWraped);
	_pEditView->wrap(isWraped);

	_pNonEditView->wrap(!isWraped);
	_pNonEditView->wrap(isWraped);

	if (toRTL)
	{
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT, SCI_CHARLEFT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_SHIFT << 16), SCI_CHARLEFTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARLEFTRECTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_CTRL << 16), SCI_WORDLEFT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDLEFTEXTEND);

		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT, SCI_CHARRIGHT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_SHIFT << 16), SCI_CHARRIGHTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARRIGHTRECTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_CTRL << 16), SCI_WORDRIGHT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDRIGHTEXTEND);

		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT, SCI_CHARLEFT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_SHIFT << 16), SCI_CHARLEFTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARLEFTRECTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_CTRL << 16), SCI_WORDLEFT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDLEFTEXTEND);

		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT, SCI_CHARRIGHT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_SHIFT << 16), SCI_CHARRIGHTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARRIGHTRECTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_CTRL << 16), SCI_WORDRIGHT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDRIGHTEXTEND);
	}
	else
	{
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT, SCI_CHARRIGHT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_SHIFT << 16), SCI_CHARRIGHTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARRIGHTRECTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_CTRL << 16), SCI_WORDRIGHT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDRIGHTEXTEND);

		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT, SCI_CHARLEFT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_SHIFT << 16), SCI_CHARLEFTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARLEFTRECTEXTEND);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_CTRL << 16), SCI_WORDLEFT);
		_pEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDLEFTEXTEND);

		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT, SCI_CHARRIGHT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_SHIFT << 16), SCI_CHARRIGHTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARRIGHTRECTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + (SCMOD_CTRL << 16), SCI_WORDRIGHT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_RIGHT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDRIGHTEXTEND);

		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT, SCI_CHARLEFT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_SHIFT << 16), SCI_CHARLEFTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_ALT) << 16), SCI_CHARLEFTRECTEXTEND);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + (SCMOD_CTRL << 16), SCI_WORDLEFT);
		_pNonEditView->execute(SCI_ASSIGNCMDKEY, SCK_LEFT + ((SCMOD_SHIFT + SCMOD_CTRL) << 16), SCI_WORDLEFTEXTEND);
	}

	if (_pDocMap)
	{
		_pDocMap->changeTextDirection(toRTL);
	}
}
break;

NOTE:

  1. I use NPP RTL but I have both views defaulted to LTR.
    With NPP original behavior, the keys would need to be re-assigned on startup as well.
  2. For my personal use I prefer to apply the direction-change to both views.
  3. You might want to shorten the code by using a function.
  4. DELETE and DELETEBACK work as expected in both directions. - No need to change anything.

@donho
Copy link
Member

donho commented Dec 28, 2021

It has been fixed in v4.1.0, according Scintilla author. However, the Scintilla used by Notepad++ is v4.4.6.
I'm not sure if it's a regression in Scintilla, or Scintilla is misused in Notepad++ for causing this bug. If it's the 2nd case, it would be better to fix it in the layer of Notepad++.

After checking the source code, I realized that Notepad++ R2L feature was done before the similar feature implemented in Scintilla - R2L feature in Notepad++ has been implemented by using Windows system's R2L capacity, but not using the bi_directional text implementation Scintilla SCI_SETBIDIRECTIONAL. That's why Neil (Scintilla author) has confirmed that has been fixed, but the fix is not in Notepad++.

The wired thing is, I have tested in the latest release of Notepad3, there is also the same caret inverse moving bug in RTL mode, but I didn't check Notepad3's code for this part.

I have also tried Scintilla native SCI_SETBIDIRECTIONAL, without success. So I took the solution of @Yaron10 and improved it as the PR #10963, which for me it's a better solution, comparing to the one of modifying Scintilla component which is not maintained by this project.

@donho donho added the reject label Dec 28, 2021
@donho donho closed this Dec 28, 2021
donho added a commit that referenced this pull request Dec 29, 2021
@Ashfaaq18 Ashfaaq18 deleted the rtlArrowFix branch August 30, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reject scintilla dependent Can't be considered for N++ implementation unless/until Scintilla changes
Projects
None yet
5 participants