Markers right click actions?

[Héctor A]

I've been looking into adding custom context menus for markers (mainly in Windows), and was a bit surprised because it seems there is no native way to do this, am I right? SCN_MARGINCLICK doesn't seem to trigger for right click events, is this the intended behaviour? 

I guess I can add the desired logic by catching typical Windows messages, then checking the margins width to see if I'm in some desired margin, then getting the corresponding line to that position and last, checking if there is a marker there. But this seems to be a cumbersome process as I think this is standard behaviour in a lot of editors, and maybe Scintilla already has built-in support for this and I missed it, or maybe I'm overcomplicating things and there is an easier way of achieving what I want.

[Neil Hodgson]

Héctor A:

> SCN_MARGINCLICK doesn't seem to trigger for right click events, is this the intended behaviour?

Yes, that is the intended behaviour.

> I guess I can add the desired logic by catching typical Windows messages, then checking the margins width to see if I'm in some desired margin, then getting the corresponding line to that position and last, checking if there is a marker there. But this seems to be a cumbersome process as I think this is standard behaviour in a lot of editors, and maybe Scintilla already has built-in support for this and I missed it, or maybe I'm overcomplicating things and there is an easier way of achieving what I want.

Adding a new notification would be an OK feature but someone would have to do the work. Such a change should preserve current behaviour unless the container asks for changes and should avoid unnecessary costs.

> and last, checking if there is a marker there.

This last part assumes a bit much about how the client will want to work and could lead to unnecessary overhead gathering information on markers. Notifying the line and column would be faster.

Neil

[Kit Yam Tse]

Hi all,

I am interested in implement this new notification. I know how to capture the right mouse down on Cocoa and Windows, however, I have no experience on GTK+. Will you accept the patch if the notification only available on these two platforms? Of course, I will take a look on GTK+ too but I can't guarantee anything.

Kit

[Neil Hodgson]

Kit Yam Tse:

> I am interested in implement this new notification. I know how to capture the right mouse down on Cocoa and Windows, however, I have no experience on GTK+. Will you accept the patch if the notification only available on these two platforms?

Yes.

> Of course, I will take a look on GTK+ too but I can't guarantee anything.

Its quite similar. Look for "event->button == 3" in ScintillaGTK::PressThis.

Neil

[Kit Yam Tse]

I captured and forward right mouse down events to scintilla, and scintilla will send SCN_MARGINRIGHTCLICK notification if the right mouse down is on the margin.

Win32 and GTK+ already listen to right mouse down for empty selection and showing content menu. I think these action should be done after UI received SCN_MARGINRIGHTCLICK, however, for backward compatibility, I kept these listener.

The new RightMouseDown method interface are as same as the exist ButtonDown methods. ButtonDown methods has a timestamp param for checking double click. I have no idea shall we track double left click, so I kept the timestamp here but do nothings in the new functions. Shall I remove this param? Or add a new SCN_DOUBLERIGHTCLICK notification?

Kit

[Kit Yam Tse]

One more thing, the patch is tested on win and mac, but not gtk and qt. Because I have no such build environment on my machine.

Kit

[johnsonj]

Please stick to use tab for indentation for sicntilla source code.
It is hard to read due to too much lines changed.

[Kit Yam Tse]

johnsonj

Thanks for your advice. I though Xcode can smartly detect and use the same indent found in the file like Atom. The GUI I used, SourceTree, to generate the .diff didn't show the differences in tab and space, so I didn't notice that the tabs are converted into spaces.

The indentations of files should be respected now.

Kit

[Neil Hodgson]

Kit Yam Tse:

> <patch.diff>

The patch applied correctly although there is a problem with ScintillaWin.cxx (discussed in last paragraph).

The <Event> methods came before the <Event>WithModifiers methods and only exist for compatibility with old platform layers. Since right click is new, there only needs to be RightButtonDownWithModifiers and no RightButtonDown. Passing a bitmask allows adding more flags in the future whereas the 3 explicit bools doesn’t.

For Qt, the raw flags should be reported. The weird rectangular case is just for dragging rectangular selections because Alt+Drag is often intercepted by X11 window managers. So drop the Q_WS_X11 segment.

Copy the indentation and brace positioning style of the file - most of Scintilla is 1TBS with tabs. Some of the platform layers were contributed by others and should follow their current style. Editor::GetMargin and Editor::RightMarginClick don’t look similar to other methods in Editor. If GetMargin is const (as it appears to be) it should be marked const. Since it looks like it only accesses ‘vs’ it may be possible to move it onto ViewStyle. Editor is too big and code should be moved out where it is sensible.

The empty braces after WM_RBUTTONDOWN aren’t correct and this file doesn’t build: there should just be an opening brace. The debug trace shouldn’t be included (and I should remove some of the others left in the file).

Neil

[Kit Yam Tse]

Neil:

On Tuesday, October 4, 2016 at 7:16:52 AM UTC+8, Neil Hodgson wrote:

Kit Yam Tse:

> <patch.diff>

   The patch applied correctly although there is a problem with ScintillaWin.cxx (discussed in last paragraph).

   The <Event> methods came before the <Event>WithModifiers methods and only exist for compatibility with old platform layers. Since right click is new, there only needs to be RightButtonDownWithModifiers and no RightButtonDown. Passing a bitmask allows adding more flags in the future whereas the 3 explicit bools doesn’t.

Removed <Event> methods, only keeps <Event>WithModifiers version

   For Qt, the raw flags should be reported. The weird rectangular case is just for dragging rectangular selections because Alt+Drag is often intercepted by X11 window managers. So drop the Q_WS_X11 segment.

Removed Q_WS_X11

   Copy the indentation and brace positioning style of the file - most of Scintilla is 1TBS with tabs. Some of the platform layers were contributed by others and should follow their current style.

Fixed diff in style 

Editor::GetMargin and Editor::RightMarginClick don’t look similar to other methods in Editor. If GetMargin is const (as it appears to be) it should be marked const. Since it looks like it only accesses ‘vs’ it may be possible to move it onto ViewStyle. Editor is too big and code should be moved out where it is sensible.

Moved the function onto view style class, and `const` is added 

   The empty braces after WM_RBUTTONDOWN aren’t correct and this file doesn’t build: there should just be an opening brace.

Fixed 

The debug trace shouldn’t be included (and I should remove some of the others left in the file).

Removed 

   Neil  

   

On the other hand, I added a checking before showing context menu. Because we should not show the context menu for document when the right mouse is down on a margin. If user wants to have context menu shown when right mouse down on a margin, then he needs to handle `SCN_MARGINRIGHTCLICK` notification.

This checking is added at Qt, GTK and Win. In Win, I need to translate the coordinate system of the point from screen to client, I have copied the translate from another function in the file, however, I haven't tested it's correctness because I am not using PC. Please verify it.

The patch has been tested on Cocoa and Qt, but not GTK or Win.

Kit

[Neil Hodgson]

Kit Yam Tse:

> On the other hand, I added a checking before showing context menu. Because we should not show the context menu for document when the right mouse is down on a margin. If user wants to have context menu shown when right mouse down on a margin, then he needs to handle `SCN_MARGINRIGHTCLICK` notification.

That is a change in behaviour which may cause problems in applications. Could be avoided by requiring the application to opt in to the new behaviour.

Perhaps by adding a 3rd state to SCI_USEPOPUP / displayPopupMenu; 2 = display popup only in text area (SC_POPUP_NEVER / SC_POPUP_ALL / SC_POPUP_TEXT).

Alternatively have the application request SCN_MARGINRIGHTCLICK with a boolean property and don’t show the popup over the margin when the application is receiving it.

> This checking is added at Qt, GTK and Win. In Win, I need to translate the coordinate system of the point from screen to client, I have copied the translate from another function in the file, however, I haven't tested it's correctness because I am not using PC. Please verify it.

It needs some casts and a PointFromPOINT conversion (POINT contains integers and Point contains floats):

POINT rpt = {static_cast<int>(pt.x), static_cast<int>(pt.y)};
::ScreenToClient(MainHWND(), &rpt);
if (!PointInSelMargin(PointFromPOINT(rpt))) {
ContextMenu(pt);
}

It shouldn’t be necessary to copy the IME code and an extra SetFocus from WM_LBUTTONDOWN. I think this can just be deleted:

// For IME, set the composition string as the result string.
IMContext imc(MainHWND());
::ImmNotifyIME(imc.hIMC, NI_COMPOSITIONSTR, CPS_COMPLETE, 0);

::SetFocus(MainHWND());


Neil

[Kit Yam Tse]

Neil,

Here is the updated patch, which introduce the pop up mode flag. However, this flag is not applied on Cocoa because the margin view doesn't ask for context menu now. Apply the flag on Cocoa may change this behaviour.

Kit

[Neil Hodgson]

Kit Yam Tse:

> Here is the updated patch, which introduce the pop up mode flag. However, this flag is not applied on Cocoa because the margin view doesn't ask for context menu now. Apply the flag on Cocoa may change this behaviour.

Currently, SciTE shows a context menu over the margin but with this patch it doesn’t. SciTE subclasses ScintillaView and implements menuForEvent: on the subclass. Perhaps SCIMarginView needs to pass the event up the super chain when the application doesn’t opt in to margin right clicks?

The displayPopupMenu test is now repeated and is likely to be repeated on other platforms not distributed from scintilla.org so could be moved up to ScintillaBase (bool ShouldDisplayPopup(pt)?):

if (displayPopupMenu == SC_POPUP_ALL ||
(displayPopupMenu == SC_POPUP_TEXT && !PointInSelMargin(rpt))) {

Neil

[Kit Yam Tse]

Neil,

`bool ShouldDisplayPopup(Point ptInWindowCoordinates)` is added to ScintillaBase. 

The coordinate system is appended to the input names in context menu related functions.  These points in different functions use different coordinate system. I think we should specify what's the coordinate system used by these function inputs.

Right click on margin view in Cocoa will show default menu, if `SCI_USEPOPUP` is set to `SC_POPUP_ALL` and the views's owner don't implement `menuForEvent:`.

In Cocoa, `ShouldDisplayPopup` is not used, `ShouldDisplayPopupOnMargin()` and `ShouldDisplayPopupOnText()` is used instead. We do already know the point is on the margin or text, and can check the context menu is enabled or not without involving the points.

Kit

[Neil Hodgson]

Kit Yam Tse:

> `bool ShouldDisplayPopup(Point ptInWindowCoordinates)` is added to ScintillaBase.
> The coordinate system is appended to the input names in context menu related functions. These points in different functions use different coordinate system. I think we should specify what's the coordinate system used by these function inputs.

The coordinate system used for the argument to ContextMenu doesn’t need to be defined by ScintillaBase since it isn’t interpreted but just tunnelled from the platform layer back to the platform layer. Whether a platform layer uses window-relative or screen-relative positions is up to the platform implementer.

The argument to ShouldDisplayPopup isn’t in window coordinates as it is negative when over the margin on Cocoa. On Cocoa, it can be thought of as relative to the text view. On most other platforms it is relative to the combined margin & text view.

> Right click on margin view in Cocoa will show default menu, if `SCI_USEPOPUP` is set to `SC_POPUP_ALL` and the views's owner don't implement `menuForEvent:`.

The check for implementation appears to have no effect in the current code or this patch. ShouldDisplayPopupOnText and ShouldDisplayPopupOnMargin are never called for me. Perhaps because NSView has an empty implementation of menuForEvent:.

Neil

[Kit Yam Tse]

Neil

   The argument to ShouldDisplayPopup isn’t in window coordinates as it is negative when over the margin on Cocoa. On Cocoa, it can be thought of as relative to the text view. On most other platforms it is relative to the combined margin & text view.

position x and y of theEvent in `- (NSMenu*) menuForEvent: (NSEvent*) theEvent` of OCIMarginView is using view coordinates related to the margin view but not the text view. If we want to use `ShouldDisplayPopup()` , then we have to do some translations on the coordinates and then send it to `ShouldDisplayPopup()`. I would like to avoid such translations so I introduce `ShouldDisplayPopupOnText()` and `ShouldDisplayPopupOnmargin()`. 

Do you want me to implement these translations and use `ShouldDisplayPopup()` instead?

 

   The check for implementation appears to have no effect in the current code or this patch. ShouldDisplayPopupOnText and ShouldDisplayPopupOnMargin are never called for me. Perhaps because NSView has an empty implementation of menuForEvent:.

You are right, they will not be called because ScintillaView always implements empty `menuForEvent:`. Perhaps we should update the implementation to something like:

```

- (NSMenu *)menuForEvent:(NSEvent *)event

{

  NSMenu *menu = [owner menuForEvent: theEvent];

  if (menu != nil) {

    return menu;

  } else if ( /* SCI_USEPOPUP == true */ ){

    return owner.backend->CreateContextMenu(theEvent);

  } else {

    return nil;

  }

}

```

Then user can choose use the menu provided by scintilla or cocoa by override the return value in owner's `menuForEvent:`

What do you think?

Kit

[Neil Hodgson]

Kit Yam Tse:

> … I would like to avoid such translations so I introduce `ShouldDisplayPopupOnText()` and `ShouldDisplayPopupOnmargin()`.

>
> Do you want me to implement these translations and use `ShouldDisplayPopup()` instead?

I think the two methods are OK.

> You are right, they will not be called because ScintillaView always implements empty `menuForEvent:`. Perhaps we should update the implementation to something like:
> ```
> - (NSMenu *)menuForEvent:(NSEvent *)event
> {
> NSMenu *menu = [owner menuForEvent: theEvent];
> if (menu != nil) {

Possibly.

It would help if Mike Lischke would respond with the reasoning behind the addition of the respondsToSelector: check in this change set:

https://sourceforge.net/p/scintilla/code/ci/93f0e2e953e69e7c99bd6691d0a2180fb2961261/

Its possible that subsequent changes have made the check ineffective.

Neil

[Neil Hodgson]

ViewStyle::MarginFromLocation looks finished so was committed:
https://sourceforge.net/p/scintilla/code/ci/92b033866ac5143035d4d8b1cc19a3ad862c073b/

Neil

[Héctor A]

Really great to see the discussion. I didn't get any notification and was nicely surprised to see it. Is there any ETA on when this could be fully implemented?

[Neil Hodgson]

Héctor A:

> Really great to see the discussion. I didn't get any notification and was nicely surprised to see it. Is there any ETA on when this could be fully implemented?

There are still some details in the most recent patch that are unclear, particularly on Cocoa where I am unsure what the maximally compatible change is.

The most recent patch changes the behaviour of ContextMenu so that it doesn’t check the displayPopupMenu variable because the checking was moved to the platform layer. This is fine for the included platforms which are all changed but won’t work for externally maintained platforms until they are updated. Restoring the check would likely make the patch OK to apply except for Cocoa.

Incompatible Scintilla releases cause bug reports, upset clients, and much more work for me so its something I try strongly to avoid.

Neil

[Kit Yam Tse]

Neil,

I dropped Mike a mail, and he said he had no idea about commit 93f0e2 and said that the commit is done by you. Do you remember anything about this commit?

Kit

[Neil Hodgson]

Kit Yam Tse:

> I dropped Mike a mail, and he said he had no idea about commit 93f0e2 and said that the commit is done by you. Do you remember anything about this commit?

The change was added and removed a couple of times but I think this is the initial addition in Mike’s repository (rev 40 on 2010-08-03) - its the first green bit when cocoa/ScintillaView.mm is expanded:

http://bazaar.launchpad.net/~mike-lischke/scintilla-cocoa/trunk/revision/40

The comment says “Context menu can now be retrieved from the owner (for customized menus) or uses that from Scintilla.”.

It is most likely that this code did not work when committed and its effect was to stop displaying the common Scintilla context menu. It is in SCIContentView and mOwner is the containing ScintillaView. Since ScintillaView does not have an implementation of menuForEvent (but its superclass NSView has a null implementation) there would only be a menu if ScintillaView has been subclassed.

Its possible that respondsToSelector was added in an attempt to avoid the cost of calling menuForEvent.

The apparent intention of the code can be restored by calling menuForEvent instead of performing a respondsToSelector check like your code:

NSMenu *menu = [owner menuForEvent: theEvent];
if (menu != nil) {

return menu;

Neil

[Kit Yam Tse]

Neil,

I got another idea on how to determine which menu should be used, scintilla one, or the platform specified one.

User may implement their own menu by subclassing ScintillaView and returning null from `- (NSMenu*) menuForEvent: (NSEvent*) theEvent` in the derived class. If we return non-nil menu from scintilla when we get the null menu from the derived class, then the application might have unexpected result.

Instead of checking null menu, we check if the user implemented `- (NSMenu*) menuForEvent: (NSEvent*) theEvent` in ScintillaView's subclass. If the mOwner implemented this function and it is not ScintillaView itself, then we will use it's implementation, otherwise, we will check `displayPopupMenu` and return the default scintilla menu or nil.

And here is the code:

```

- (NSMenu*) menuForEvent: (NSEvent*) theEvent

{

  if ([mOwner methodForSelector:@selector(menuForEvent:)] != [[ScintillaView class] instanceMethodForSelector:@selector(menuForEvent:)]) {

    // menuForEvent Overriden

    return [mOwner menuForEvent: theEvent];

  } else if (mOwner.backend->ShouldDisplayPopupOnText()) {

    return mOwner.backend->CreateContextMenu(theEvent);

  } else {

    return nil;

  }

}

```

By the way, ShouldDisplayPopupOnText , should be true here by default on Cocoa at my patch. If we don't want to have the menu by default in Cocoa, then we can set `SCI_USEPOPUP` to `SC_POPUP_NEVER` when we init ScintillaView.

Any comment?

Kit

[Mike Lischke]

>
>> I dropped Mike a mail, and he said he had no idea about commit 93f0e2 and said that the commit is done by you. Do you remember anything about this commit?
>
> The change was added and removed a couple of times but I think this is the initial addition in Mike’s repository (rev 40 on 2010-08-03) - its the first green bit when cocoa/ScintillaView.mm is expanded:
>
> http://bazaar.launchpad.net/~mike-lischke/scintilla-cocoa/trunk/revision/40
>
> The comment says “Context menu can now be retrieved from the owner (for customized menus) or uses that from Scintilla.”.
>
> It is most likely that this code did not work when committed and its effect was to stop displaying the common Scintilla context menu.

The class structure at that time, when I wrote this code, was different. In the meantime that has changed, hence code like the context menu handling might need an update.

> It is in SCIContentView and mOwner is the containing ScintillaView. Since ScintillaView does not have an implementation of menuForEvent (but its superclass NSView has a null implementation) there would only be a menu if ScintillaView has been subclassed.

Which is the main idea behind that code. In my app ScintillaView is subclassed connect it with the rest of the application, including providing an own menu.

>
> Its possible that respondsToSelector was added in an attempt to avoid the cost of calling menuForEvent.

No, it's been added to allow owners (including the plain ScintillaView class) not to provide custom menus, but instead use the one from the underlying Scintilla backend. So, it's a compatibility feature.

>
> The apparent intention of the code can be restored by calling menuForEvent instead of performing a respondsToSelector check like your code:
> NSMenu *menu = [owner menuForEvent: theEvent];
> if (menu != nil) {
> return menu;

Now with that changed structure this would certainly work, since we have that NSView base class. Makes it even simpler than before.

Mike
--
www.soft-gems.net

[Mike Lischke]

I didn't follow this discussion, so bear with me if I got something wrong.

And here is the code:

```

- (NSMenu*) menuForEvent: (NSEvent*) theEvent

{

  if ([mOwner methodForSelector:@selector(menuForEvent:)] != [[ScintillaView class] instanceMethodForSelector:@selector(menuForEvent:)]) {

    // menuForEvent Overriden

    return [mOwner menuForEvent: theEvent];

  } else if (mOwner.backend->ShouldDisplayPopupOnText()) {

    return mOwner.backend->CreateContextMenu(theEvent);

  } else {

    return nil;

  }

}

```

By the way, ShouldDisplayPopupOnText , should be true here by default on Cocoa at my patch. If we don't want to have the menu by default in Cocoa, then we can set `SCI_USEPOPUP` to `SC_POPUP_NEVER` when we init ScintillaView.

Why is that ShouldDisplayPopupOnText necessary at all? Simply return nil if you don't want to show a menu. You can do as Neil suggested and always call menuForEvent, which can be overwritten by descendants to return a non-nil value (if that's wanted). If nil is returned call CreateContextMenu() instead. That can then return a menu or not, whatever it is set to do. This makes the platform code work transparently over any backend change.

Mike
-- 
www.soft-gems.net

[Neil Hodgson]

Mike Lischke:

> Why is that ShouldDisplayPopupOnText necessary at all? Simply return nil if you don't want to show a menu.

Scintilla has a cross-platform SCI_POPUP API that turns off Scintilla’s basic context menu. This proposed feature refines that so that there is an additional state where the popup still occurs over the text but not over the margin so that an event is instead sent to the application. Application code can then perform an action which may be to display a context menu over the margin but it could also be something like adding a bookmark.

Neil

[Neil Hodgson]

Kit Yam Tse:

> User may implement their own menu by subclassing ScintillaView and returning null from `- (NSMenu*) menuForEvent: (NSEvent*) theEvent` in the derived class. If we return non-nil menu from scintilla when we get the null menu from the derived class, then the application might have unexpected result.

On other platforms there is always a basic context menu provided by Scintilla and this has worked in the past on Cocoa. Its also common for edit widgets and should not often be a problem. There could be applications which want to disable access to some commands (such as ‘paste’, for example) but they can call SCI_USEPOPUP.

I think Scintilla on Cocoa should display a context menu unless it has been switched off with SCI_USEPOPUP or the ScintillaView subclass returns a non-null menu.

> Instead of checking null menu, we check if the user implemented `- (NSMenu*) menuForEvent: (NSEvent*) theEvent` in ScintillaView's subclass. If the mOwner implemented this function and it is not ScintillaView itself, then we will use it's implementation, otherwise, we will check `displayPopupMenu` and return the default scintilla menu or nil.
>
> And here is the code:
> ```
> - (NSMenu*) menuForEvent: (NSEvent*) theEvent
> {
> if ([mOwner methodForSelector:@selector(menuForEvent:)] != [[ScintillaView class] instanceMethodForSelector:@selector(menuForEvent:)]) {

This appears overly specific to me and could break if the application inserts its menu logic in a less common way such as using message forwarding. Calling menuForEvent on mOwner gives the subclass an opportunity to provide a menu without requiring it to implement this in a specific way. It should also be reasonably performant as returning null is quick and its only in the case where the subclass wants a menu that it has to perform allocations.

If this was new code, I’d argue in favour of checking SCI_USEPOPUP before [mOwner menuForEvent: theEvent] but current applications are expecting the menuForEvent.

So:

NSMenu *menu = [mOwner menuForEvent: theEvent];
if (menu) {
return menu;

} else if (mOwner.backend->ShouldDisplayPopupOnText()) {
return mOwner.backend->CreateContextMenu(theEvent);
} else {
return nil;
}

Neil

[Neil Hodgson]

Here is a patch with menuForEvent updated, the already committed ViewStyle::MarginFromLocation removed, ScintillaBase::ContextMenu reverted for compatibility with external platform layers, and some other minor reversions like renaming ptInScreenCoordinates back to pt and some whitespace reverted.

Neil

[Neil Hodgson]

Me:

> Here is a patch with menuForEvent updated, the already committed ViewStyle::MarginFromLocation removed, ScintillaBase::ContextMenu reverted for compatibility with external platform layers, and some other minor reversions like renaming ptInScreenCoordinates back to pt and some whitespace reverted.

This has been committed as
https://sourceforge.net/p/scintilla/code/ci/e4306dd383438c0a149d4c2abee9d53a1486a9ea/
Some additional changes were made to the Win32 code to prevent problems with declaring variables inside a switch and ScintillaBase::ShouldDisplayPopup was made const.

Neil

[Neil Hodgson]

On GTK+, restore ability for application to handle a right click. This was removed by the right margin click implementation which returned TRUE instead of FALSE like the previous release.
https://sourceforge.net/p/scintilla/code/ci/124147f7a8b90aecee6b917c60b4198138345b6d/


Neil