Hopefully not to late for 2.03... hotspot bug fix patch
almostautomated
https://sourceforge.net/tracker/?func=detail&aid=2951353&group_id=2439&atid=102439
Added a patch to the tracker for a simple (but annoying) little
hotspot bug, where it doesn't clear when mouse exits out of client
area from the hostspot.
Hopefully it can make it into 2.03
Thanks,
Thell
========
diff --git a/scintilla/src/Editor.cxx b/scintilla/src/Editor.cxx
index 6016f16..35f3c9c 100644
--- a/scintilla/src/Editor.cxx
+++ b/scintilla/src/Editor.cxx
@@ -5842,6 +5842,7 @@ void Editor::ButtonMove(Point pt) {
if (vs.fixedColumnWidth > 0) { // There is a margin
if (PointInSelMargin(pt)) {
DisplayCursor(Window::cursorReverseArrow);
+ SetHotSpotRange(NULL);
return; // No need to test for
selection
}
}
diff --git a/scintilla/win32/ScintillaWin.cxx b/scintilla/win32/
ScintillaWin.cxx
index ac4ef53..793cdef 100644
--- a/scintilla/win32/ScintillaWin.cxx
+++ b/scintilla/win32/ScintillaWin.cxx
@@ -780,6 +780,7 @@ sptr_t ScintillaWin::WndProc(unsigned int
iMessage, uptr_t wParam, sptr_t lParam
}
return TRUE;
} else {
+ SetHotSpotRange(NULL);
return ::DefWindowProc(MainHWND(),
iMessage, wParam, lParam);
}
Neil Hodgson
> https://sourceforge.net/tracker/?func=detail&aid=2951353&group_id=2439&atid=102439
>
> Added a patch to the tracker for a simple (but annoying) little
> hotspot bug, where it doesn't clear when mouse exits out of client
> area from the hostspot.
The patch doesn't really line up with current CVS so I have to
guess where the chunks are applied. The first chunk drops the hot spot
when the mouse moves over the margin. This is OK.
The second patch appears to change the hot spot in the WM_SETCURSOR
handler. The only thing that should happen in WM_SETCURSOR is setting
the cursor. This functionality belongs in ButtonMove so that it is
cross-platform with possibly a TrackMouseEvent/WM_MOUSELEAVE or Tick
handler to detect movement outside the window.
> Hopefully it can make it into 2.03
Several days too late. Once a plan for a new release is announced,
only regressions from the previous release, very serious bugs and
inconsequential changes (like documentation) are accepted.
Neil
almostautomated
--- snip --->8
> The second patch appears to change the hot spot in the WM_SETCURSOR
> handler. The only thing that should happen in WM_SETCURSOR is setting
> the cursor. This functionality belongs in ButtonMove so that it is
> cross-platform with possibly a TrackMouseEvent/WM_MOUSELEAVE or Tick
> handler to detect movement outside the window.
>
Completely understood. How about altering ScintillaWin::WinProc to
include
a non-client mouse move to clear the HotSpot then forward the message
as it
does now?
case WM_MOUSEMOVE:
ButtonMove(Point::FromLong(lParam));
break;
case WM_NCMOUSEMOVE:
SetHotSpotRange(NULL);
return ::DefWindowProc(MainHWND(), iMessage, wParam, lParam);
At first I thought having WM_NCMOUSEMOVE call ButtonMove() would be a
good way
to go, yet since h NC message already tests for being within the
client area.... Another
option could be to have::
case WM_MOUSEMOVE:
case WM_NCMOUSEMOVE:
ButtonMove(Point::FromLong(lParam));
break;
And alter ButtonMove to check if point is in the client area and
ptMouseLast was a hotspot:
if (inDragDrop == ddInitial) {
if (DragThreshold(ptMouseLast, pt)) {
SetMouseCapture(false);
SetDragPosition(movePos);
CopySelectionRange(&drag);
StartDrag();
}
return;
}
if (PointIsHotspot(ptMouseLast)) {
PRectangle rcClient = GetClientRectangle();
if (pt.y <= rcClient.top || pt.y >= rcClient.bottom ||
pt.x <= rcClient.left || pt.x >= rcClient.right) {
SetHotSpotRange(NULL);
return;
}
}
ptMouseLast = pt;
Option 1 seems cleaner, IMHO, but I may not be seeing the big
picture...
Thanks again,
Thell
Neil Hodgson
> Completely understood. How about altering ScintillaWin::WinProc to
> include a non-client mouse move to clear the HotSpot then forward
> the message as it does now?
Scintilla is often frameless so the only non-client edges are the
right and bottom where the scroll bars are. To demonstrate this, use
Spy++ and SciTE. Turn on just the non-client messages in Spy++ and try
to produce a WM_NCMOUSEMOVE off the top edge. I can't using Windows
XP.
I think mouse move events are sampled so you can't depend on
getting one if the mouse is moved quickly over a frame. This is
probably why TrackMouseEvent was added.
According to MSDN, TrackMouseEvent was introduced in Windows 98, so
if you are going to use it, it should be explicitly found like
AlphaBlend is.
Neil
almostautomated
On Feb 14, 6:31 pm, Neil Hodgson <nyamaton...@gmail.com> wrote:
> almostautomated:
>
> > Completely understood. How about altering ScintillaWin::WinProc to
> > include a non-client mouse move to clear the HotSpot then forward
> > the message as it does now?
>
> Scintilla is often frameless so the only non-client edges are the
> right and bottom where the scroll bars are. To demonstrate this, use
> Spy++ and SciTE. Turn on just the non-client messages in Spy++ and try
> to produce a WM_NCMOUSEMOVE off the top edge. I can't using Windows
> XP.
>
> I think mouse move events are sampled so you can't depend on
> getting one if the mouse is moved quickly over a frame. This is
> probably why TrackMouseEvent was added.
>
Right on both counts. Added TrackMouseEvent to ScintillaWin and it
works
with on a frameless editor.
> According to MSDN, TrackMouseEvent was introduced in Windows 98, so
> if you are going to use it, it should be explicitly found like
> AlphaBlend is.
>
LOL, keeping support all the way back eh? How about using
DllGetVersion?
Neil Hodgson
> LOL, keeping support all the way back eh?
Its not worth breaking compatibility for something not needed by
all. If Scintilla does break compatibility at some point it will
probably be for a more fundamental change like dropping all
non-Unicode releases of Windows (95, 98, ME).
> How about using DllGetVersion?
Its better to test for features rather than versions. IIRC the
AlphaBlend function was made available with a release of Internet
Explorer as well as new releases of Windows.
Neil
almostautomated
On Feb 15, 3:36 pm, Neil Hodgson <nyamaton...@gmail.com> wrote:
> almostautomated:
>
> > LOL, keeping support all the way back eh?
>
> ... If Scintilla does break compatibility at some point it will
> probably be for a more fundamental change like dropping all
> non-Unicode releases of Windows (95, 98, ME).
>
I'd be all for that.
> > How about using DllGetVersion?
>
> Its better to test for features rather than versions. IIRC the
> AlphaBlend function was made available with a release of Internet
> Explorer as well as new releases of Windows.
>
I've attached a new diff to the tracker item (v2). I know it doesn't
line up with the official source, and for that I apologize. Hopefully
this is getting closer. It works fine on my Win7 system, but I don't
have access to a Win95 system to test on...
Let me know.
Thell
=== The following isexplanation of what was done ===
Added the following to ScintillaWin.cxx:
typedef BOOL (WINAPI *TrackMouseEventSig)(LPTRACKMOUSEEVENT);
static TrackMouseEventSig TrackMouseEventFn = 0;
Then added to class ScintillaWin :
bool trackedMouseLeave;
virtual void SetTrackMouseLeaveEvent(bool on);
Which function is defined as:
void ScintillaWin::SetTrackMouseLeaveEvent(bool on) {
if (on && TrackMouseEventFn && !trackedMouseLeave) {
TRACKMOUSEEVENT tme;
tme.cbSize = sizeof(tme);
tme.dwFlags = TME_LEAVE;
tme.hwndTrack = MainHWND();
TrackMouseEventFn(&tme);
}
trackedMouseLeave = on;
}
Added to : ScintillaWin::ScintillaWin(HWND hwnd) {
trackedMouseLeave = false;
Added to void ScintillaWin::Initialise() :
HMODULE user32 = GetModuleHandle ("user32.dll");
TrackMouseEventFn = (TrackMouseEventSig)::GetProcAddress (user32,
"TrackMouseEvent");
if (TrackMouseEventFn == NULL) {
HMODULE commctrl32 = LoadLibrary ("commctrl32.dll");
if (commctrl32 != NULL) {
TrackMouseEventFn = (TrackMouseEventSig)
::GetProcAddress (commctrl32, "_TrackMouseEvent");
}
}
then in ScintillaWin::WndProc()
case WM_MOUSEMOVE:
SetTrackMouseLeaveEvent(true);
ButtonMove(Point::FromLong(lParam));
break;
case WM_MOUSELEAVE:
SetTrackMouseLeaveEvent(false);
Neil Hodgson
> typedef BOOL (WINAPI *TrackMouseEventSig)(LPTRACKMOUSEEVENT);
> static TrackMouseEventSig TrackMouseEventFn = 0;
To avoid any possibility of sharing/threading problems, moved
TrackMouseEventFn into ScintillaWin object.
> HMODULE commctrl32 = LoadLibrary ("commctrl32.dll");
Should be comctl32.dll. This works on Windows XP if the user32
search is removed.
> case WM_MOUSELEAVE:
> SetTrackMouseLeaveEvent(false);
> SetHotSpotRange(NULL);
Moving SetHotSpotRange(NULL); into Editor class as MouseLeave and
calling MouseLeave here.
void Editor::MouseLeave() {
SetHotSpotRange(NULL);
}
This allows other platforms to call MouseLeave() and for all common
functionality to be implemented in it.
Neil
almostautomated
I looked on CVS and didn't see a commit for this, could you post a
diff/patch to the tracker of your changes so I can test on Win7?
Thanks
Thell
Neil Hodgson
> I looked on CVS and didn't see a commit for this, could you post a
> diff/patch to the tracker of your changes so I can test on Win7?
Its not committed yet as I'm still a little unsure.
Diff attached.
Neil
joce
I reviewed that change for inclusion in Notepad++CR:
http://nppcommunity.lighthouseapp.com/projects/38932-npp-community/tickets/16-bug-sticky-hotspot-style#ticket-16-7
Here are my comments.
* I don't have commctrl32.dll on my machine. However, when I went
looking for it online, I found that it's most probably comctrl32.dll
that you are referring to (especially since it contained the function
you want to get the address from).
* I'd personally put the whole dll loading block in its own
method. Actually, when I went looking for it, I found
DynamicLibraryImpl in win32\PlatWin.cxx. This probably should be made
use of.
* The parameter name in ScintillaWin::SetTrackMouseLeaveEvent(bool
on) is terrible. To 99.9% of humans, "on" means... well, on. It
represents positive, 1, true in people's mind. I'd use something more
like "state" or something along those lines, or even better, an enum
with even more sense: enum TrackState{ Tracked, Untracked };.
I understand that point #1 is fixed, but I fee strongly about point
#2. And point #3 if not fixed leaves the code in a state that is hard
to understand and therefore bad.
Comment?
joce.
> leave.diff
> 6KViewDownload
joce
tracking function would benefit from the use of the DynamicLibraryImpl
class:
TrackMouseEventFn =
DynamicLibraryImpl("comctrl32.dll").FindFunction("_TrackMouseEvent");
That's extra clean and makes use of reusable code.
joce.
> leave.diff
> 6KViewDownload
Neil Hodgson
> * I don't have commctrl32.dll on my machine. However, when I went
> looking for it online, I found that it's most probably comctrl32.dll
Its comctl32.dll - one 'm', no 'r'. It should be present on all
versions of Windows since Windows 95.
> * I'd personally put the whole dll loading block in its own
> method. Actually, when I went looking for it, I found
> DynamicLibraryImpl in win32\PlatWin.cxx. This probably should be made
> use of.
DynamicLibraryImpl is the hidden implementation part of the public
DynamicLibrary class. It should never be directly instantiated. The
purpose of DynamicLibrary is to provide platform-independent access to
dynamic libraries from platform-independent code.
Since the code that finds TrackMouseEvent is in a Win32-specific
module its more direct and obvious to call the Win32 API.
> * The parameter name in ScintillaWin::SetTrackMouseLeaveEvent(bool
> on) is terrible. To 99.9% of humans, "on" means... well, on. It
> represents positive, 1, true in people's mind. I'd use something more
> like "state" or something along those lines, or even better, an enum
> with even more sense: enum TrackState{ Tracked, Untracked };.
The name 'on' is fine. Its a boolean saying whether something is
on. Its a convention used in other aspects of the Scintilla API.
Adding an enum for a single piece of state that is internal to just
the ScintillaWin class is adding extra names and thus complexity.
Neil
joce
> > method. Actually, when I went looking for it, I found
> > DynamicLibraryImpl in win32\PlatWin.cxx. This probably should be made
> > use of.
>
> DynamicLibraryImpl is the hidden implementation part of the public
> DynamicLibrary class. It should never be directly instantiated. The
> purpose of DynamicLibrary is to provide platform-independent access to
> dynamic libraries from platform-independent code.
>
> Since the code that finds TrackMouseEvent is in a Win32-specific
> module its more direct and obvious to call the Win32 API.
You have an excellent piece of reusable code that allows for nice and
clean implementation and you prefer this single use glob of code
instead? I'm floored.
> Adding an enum for a single piece of state that is internal to just
> the ScintillaWin class is adding extra names and thus complexity.
>
Really?
Between
Func( true, false, true );
and
Func( ApplyDynamics, IgnoreInputPin, UsePosition );
You'd rather choose the first one? Don't you see it's just terse? It
doesn't mean anything!
And it's not like it's an entirely new concept to do this. I've been
applying this concept since I read Writing Solid Code: "Make the code
intelligible at the point of call. Avoid boolean arguments." (Writing
Solid Code, Steve Maguire, 1993, pages 102 - 104, ISBN 1-55615-551-4).
This is an advice that is almost 20 years old and that IMO still
stand.
joce.
joce
> > method. Actually, when I went looking for it, I found
> > DynamicLibraryImpl in win32\PlatWin.cxx. This probably should be made
> > use of.
>
> DynamicLibraryImpl is the hidden implementation part of the public
> DynamicLibrary class. It should never be directly instantiated. The
> purpose of DynamicLibrary is to provide platform-independent access to
> dynamic libraries from platform-independent code.
>
> Since the code that finds TrackMouseEvent is in a Win32-specific
> module its more direct and obvious to call the Win32 API.
You have an elegant piece of reusable code that allows for nice and
clean implementation and you prefer this single use glob of code
instead? I'm floored.
> Adding an enum for a single piece of state that is internal to just
> the ScintillaWin class is adding extra names and thus complexity.
>
Really?
Neil Hodgson
> You have an excellent piece of reusable code that allows for nice and
> clean implementation and you prefer this single use glob of code
> instead? I'm floored.
Code should not be shared just because it performs a similar set of
actions. It needs to share enough of a common purpose that it is
likely to work and continue working as it is maintained.
Your preferred replacement fails. On earlier versions of Windows it
may have crashed the application. Testing it on my Windows XP
installation produces error 0x000003e6: Invalid access to memory
location.
Fixing up the misspellt name, the expression you want to use is
TrackMouseEventFn =
DynamicLibraryImpl("comctl32.dll").FindFunction("_TrackMouseEvent");
This creates a DynamicLibraryImpl object, retrieves a pointer to
_TrackMouseEvent, and destroys the DynamicLibraryImpl object. In the
destructor of the DynamicLibraryImpl object, the library is freed by a
cal to FreeLibrary. Thus TrackMouseEventFn points into deallocated
memory and calling it should fail. Since this is a common bug, Windows
arranges things so that the global error is set but no crash occurs
(on XP). However, you will not get a MouseLeave notification. If you
are unlucky, another DLL may get loaded into this space and a crash
then occur.
The problem isn't with DynamicLibraryImpl - it is doing its job as
designed within the set of constraints appropriate for loading user
written extensions. The problem is that you have tried to use this
code in a different context just becasue it does something similar to
what you want.
> Really?
> Between
>
> Func( true, false, true );
>
> and
>
> Func( ApplyDynamics, IgnoreInputPin, UsePosition );
You are unfairly exaggerating the code. The actual code looks like this:
SetTrackMouseLeaveEvent(true)
> You'd rather choose the first one? Don't you see it's just terse? It
> doesn't mean anything!
I don't like either of your choices since they do not apply to the
situation in question. I am choosing SetTrackMouseLeaveEvent(true).
> And it's not like it's an entirely new concept to do this. I've been
> applying this concept since I read Writing Solid Code: "Make the code
> intelligible at the point of call. Avoid boolean arguments." (Writing
> Solid Code, Steve Maguire, 1993, pages 102 - 104, ISBN 1-55615-551-4).
> This is an advice that is almost 20 years old and that IMO still
> stand.
Its useful in some cases. Not in this case. You should not blindly
apply recommendations as if they are rules. Avoid over-engineering
code when it expands the set of entities and produces insufficient
benefit to justify that expansion. A boolean argument matching a
boolean piece of state is all that is needed here.
Neil
Jocelyn Legault
Thanks for taking the time to answer.
joce.
--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To post to this group, send email to scintilla...@googlegroups.com.
To unsubscribe from this group, send email to scintilla-inter...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/scintilla-interest?hl=en.
almostautomated
> leave.diff
> 6KViewDownload
What can I do to help make it code that you are more sure about? Even
though we are not using the latest Scintilla and we have custom
patches on it already, we'd like to not patch ours until knowing one
way or the other HotSpot style clearing getting into the official
build.
I kinda understand the hesitation though, since HotSpot clearing on
mouse leave could be viewed as the container apps duty, and it _is_ a
windows only solution.
Also, you mentioned having taken the user32.dll search out, yet it is
in the diff?
Thanks,
Thell
Neil Hodgson
> What can I do to help make it code that you are more sure about?
The thing that struck me as potentially troublesome is that
TrackMouseEvent is responsible for 2 different pieces of
functionality: mouse leaving and mouse hovering. Now, Scintilla has
its own version of hover called "dwell" but its possible that
Scintilla could move to (optionally) using Windows hover. Looking more
closely at TrackMouseEvent shows that the two aspects are separate and
shouldn't interfere.
A potential incompatibility is if an application has called
TrackMouseEvent itself, perhaps as part of subclassing Scintilla. I
think we are just going to have to accept that in the interest of
increasing functionality.
> I kinda understand the hesitation though, since HotSpot clearing on
> mouse leave could be viewed as the container apps duty, and it _is_ a
> windows only solution.
Its actually a little easier on GTK+ where you always get mouse
entry and leave events but I'll leave that to someone who needs the
functionality to implement.
> Also, you mentioned having taken the user32.dll search out, yet it is
> in the diff?
I just removed the search of user32.dll temporarily for testing the
search of comctl32.dll since the alternative was getting my Windows 95
machine out of storage. Since USER32 is already loaded,
GetModuleHandle("user32.dll") should be faster and use less memory
than calling LoadLibrary("comctl32.dll").
I'm currently fine with the patch so if no further issues are
raised soon, I'll commit it.
Neil
almostautomated
> A potential incompatibility is if an application has called
> TrackMouseEvent itself, perhaps as part of subclassing Scintilla. I
> think we are just going to have to accept that in the interest of
> increasing functionality.
>
Possibly a SCI_ GET and SET MOUSETRACK ?
> I'm currently fine with the patch so if no further issues are
> raised soon, I'll commit it.
>
That's good news. Thank you!.
Neil Hodgson
>> A potential incompatibility is if an application has called
>> TrackMouseEvent ..
>
> Possibly a SCI_ GET and SET MOUSETRACK ?
One purpose of the mailing list is so that Scintilla users can
object to changes that will be incompatible with their projects. If it
does turn out to be a problem for anyone then an API can be added.
I have committed the change.
Neil