Hotspot mousedown callback not being executed

Posted by Twisol on Mon 23 Nov 2009 07:36 AM — 29 posts, 93,785 views.

Twisol USA Mon 23 Nov 2009 07:36 AM #0

(MUSHclient v4.43)

I am writing a test case for my CharacterGrid widget type, and I'm using three of the hotspot callbacks. While two of them work great - mouseup and mouseover - the third, cancelmouseover, doesn't appear to be executing at all. However I am perfectly able to get its name from WindowHotspotInfo and execute it directly [1].

Here is the relevant snippet of code that I used:


CellFromHotspot = function(id)
  local _, _, x, y = string.find(id, "-h%((%d+),(%d+)%)")
  return grid:Cell(tonumber(x), tonumber(y))
end

CellBlur = function(flags, id)
  local cell = CellFromHotspot(id)
  
  cell.backcolor = 0xFFFFFF
  
  grid:Draw(true)
end

OnPluginInstall = function()
  grid = CharacterGrid.new(3, 3)
  grid:Anchor(12)
  grid:Font(TICTACTOE_FONT, 15)
  grid.backcolor = 0xFFFFFF
  
  for y = 1, 3 do
    for x = 1, 3 do
      grid:Cell(x, y).hotspot = {
        mouseup = "CellClick",
        mouseover = "CellFocus",
        cancelmouseover = "CellBlur",
      }
    end
  end
end

This uses my under-development widget framework [2]. The per-cell .hotspot entry maps directly to a WindowAddHotspot() call, and I can verify that there are no typos or mistakes in the code along the way [1]. (There was a typo in my passing of cancelmousedown fields, but it was unrelated and I fixed it.)

I jumped into my copy of the MUSHclient source, and it appears to me that there are two problems surrounding cancelmouseover. Firstly, it appears to only even be considered if the mouse leaves the miniwindow, not the hotspot [3]. Further, even then it doesn't appear to move past a "did we mouseover this hotspot" check.

[1] This code works as expected, calling the nominated cancelmouseover function.


name = "03829336f30e9f2dcace86e9:w1"
hotspot = "03829336f30e9f2dcace86e9:w1-h(2,2)"

_G[WindowHotspotInfo(name, hotspot, 6)](0, hotspot)

[2] http://github.com/Twisol/MUSHclient-MWidget

[3] The relevant MUSHclient source code begins at line 6393 in mushview.cpp, method CMUSHView::Mouse_Move_MiniWindow.

WillFa USA Mon 23 Nov 2009 04:20 PM #1

Looking at what's on github...

The only WindowShow is in Window.lua's draw routine (base.Draw). The way things cascade you're doing:

WindowCreate
WindowShow
WindowText
WindowText
WindowText
WindowText
WindowText
...

Does this actually display anything that's drawn ever?

Twisol USA Mon 23 Nov 2009 05:14 PM #2

Yes - the plugin itself calls Show(), either directly or through :Draw(true) (the parameter saying "Display -now-").

If it wasn't displaying things, I daresay my problems would be rather different. If you'd like, you can download the framework yourself and test it out. Here's the plugin I'm writing - but please note that it's not done yet.

EDIT: You'll have to uncomment the mouseover/cancelmouseover lines if you want to check that specific issue.


<?xml version="1.0" encoding="iso-8859-1"?>
<!DOCTYPE muclient>

<muclient>
<plugin
   name="TicTacToe"
   author="Soludra"
   id="03829336f30e9f2dcace86e9"
   language="Lua"
   purpose="Play a game of Tic Tac Toe!"
   date_written="2009-11-22 07:46:00"
   requires="4.43"
   version="1.0"
/>

<!--  Triggers  -->

<triggers>
</triggers>

<!--  Aliases  -->

<aliases>
  <alias
   match="^ttt=(.*)$"
   enabled="y"
   regexp="y"
   send_to="12"
   ignore_case="y"
   sequence="100"
  >
  <send>print(%1)</send>
  </alias>
</aliases>

<!--  Script  -->

<script>
<![CDATA[

require('tprint')

CharacterGrid = require("MWidget\\CharacterGrid")

-- Defines the default grid font.
TICTACTOE_FONT = "Lucida Console"

players = {
  [1] = {
    piece = 'X',
    color = 0x00FF00,
    lastmove = nil,
  },
  [2] = {
    piece = 'O',
    color = 0x0000FF,
    lastmove = nil,
  },
}
current_player = 1


CellFromHotspot = function(id)
  local _, _, x, y = string.find(id, "-h%((%d+),(%d+)%)")
  return grid:Cell(tonumber(x), tonumber(y))
end

CellClick = function(flags, id)
  local cell = CellFromHotspot(id)
  
  if cell.char ~= " " then
    return
  end
  
  if players[current_player].lastmove then
    players[current_player].lastmove.backcolor = 0xFFFFFF
  end
  
  cell.char = players[current_player].piece
  cell.backcolor = players[current_player].color
  
  players[current_player].lastmove = cell
  current_player = (current_player%2) + 1
  grid:Draw(true)
end

CellFocus = function(flags, id)
  local cell = CellFromHotspot(id)

  cell.backcolor = 0xFF0000
  
  grid:Draw(true)
end

CellBlur = function(flags, id)
  local cell = CellFromHotspot(id)
  
  cell.backcolor = 0xFFFFFF
  
  grid:Draw(true)
end

Reset = function()
  grid:ResetGrid()
  
  for y = 1, 3 do
    for x = 1, 3 do
      local cell = grid:Cell(x, y)
      cell.forecolor = 0x000000
      cell.hotspot = {
        mouseup = "CellClick",
        -- mouseover = "CellFocus",
        -- cancelmouseover = "CellBlur",
      }
    end
  end
  
  grid:Draw(true)
end

OnPluginInstall = function()
  grid = CharacterGrid.new(3, 3)
  grid:Anchor(12)
  grid:Font(TICTACTOE_FONT, 15)
  grid.backcolor = 0xFFFFFF

  Reset()
end

OnPluginClose = function()
  grid:Destroy()
end

OnPluginEnable = OnPluginInstall
OnPluginDisable = OnPluginClose

]]>
</script>
</muclient>

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 07:08 PM #3

Is this fixed yet? I see on your website a fix entitled "Fixed misspelling of 'cancel' ...".

My test shows that my test case works (I haven't tested yours).

Quote:

... cancelmouseover, doesn't appear to be executing at all ... and it appears to me that there are two problems surrounding cancelmousedown ...

Well, you are talking about cancelmouseover, not cancelmousedown, right?

At line 6447 in the (MUSHclient) code there is some code that appears to handle that, starting:


    // cancel previous move-over hotspot (in this miniwindow)

In my test you get a debugging display correctly once you move the mouse away from a hotspot. Otherwise my inventory and map plugins wouldn't have worked.

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 07:27 PM #4

In your code for Draw:


 Draw = function(self, show)
    local flags = self.flags or 0
    if self.position.absolute then
      flags = bit.bor(flags, 2)
    end
    
    WindowCreate(self.name, self.position.x, self.position.y,
	     self.width, self.height, self.position.anchor, flags, self.backcolor)
    if show then
      WindowShow(self.name, true)
    end
  end,

Why do you call WindowCreate in a draw function? This will make a new miniwindow, deleting existing hotspots. In the code for CMiniWindow::Create it says:


  // a newly created window has no hotspots
  DeleteAllHotspots ();

See: http://www.gammon.com.au/mushclient/mw_creation.htm

I quote from there:

Miniwindow Creation documentation said:

What happens when I re-create an existing miniwindow?

You can do a WindowCreate on an existing miniwindow with no problems. This will have the following effects:

The existing offscreen image will be discarded, and the memory it used freed.
A new offscreen image will be created, using the new dimensions. Thus you can use WindowCreate to resize a window. For example, you might do this for an inventory window, if you get more inventory.
The image will be cleared to the background colour you specify.
Any hotspots you added are removed. The assumption is that if you are starting drawing from scratch, any hotspot items will probably appear in a different place.
The window's "show" flag is set to false. Thus you need to call WindowShow before it will become visible.

The following items are retained:

Any installed fonts (see WindowFont)
Any installed images (see WindowLoadImage and WindowCreateImage)

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 07:31 PM #5

I have amended the help for WindowCreate to make this more explicit. However the existing help does refer you to the above web page with the extra information.

WillFa USA Mon 23 Nov 2009 07:47 PM #6

The bug happens because your hotspots overlap. :)


      WindowAddHotspot(self.name, self.name .. "-h(" .. x .. "," .. y .. ")",
         left, top, left+self.fonts["f"].width-1, top+self.fonts["f"].height-1,
         cell.hotspot.mouseover, cell.hotspot.cancelmouseover,
         cell.hotspot.mousedown, cell.hotspot.cancelmousedown,
         cell.hotspot.mouseup, cell.hotspot.tooltip,
         cell.hotspot.cursor, 0)

fixes the problem.

WillFa USA Mon 23 Nov 2009 08:14 PM #7

Re: Why call WindowCreate in a draw function...

It's easier to draw the whole miniwindow again instead of keeping state on what needs to be updated. And you point out that reCreating the window is a reset button that doesn't nuke fonts/images....

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 08:43 PM #8

But it *does* nuke hotspots...

WillFa USA Mon 23 Nov 2009 09:17 PM #9

what happens if you nuke a hotspot in the middle of a mousedown event script?

I can really only see it as a problem if you're doing animation for a button click effect.

OHHHHHHHH... I get it... You're never getting a cancelMouseOver because you're nuking the hotspot in the MouseOver.

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 10:38 PM #10

Indeed. You can't really complain about not getting a mouse-over cancel for a hotspot that has been deleted. Even if you recreate it later. Part of adding a hotspot clears any "memory" of what the previous hotspot (of the same name) was doing. Otherwise if you deleted a hotspot, and added it later on, somewhere else, it might still give a mouse-over cancel at an inappropriate time.

Basically, this isn't a bug. It is working as documented.

As a workaround either don't re-create the window every time you want to draw in it (after all, then you have to re-add all hotspots, and you have other problems like the one this thread is about, and it is inefficient in that memory is being freed and then reallocated), or if you must have the creation code there, do a check first that the window does not already exist of the correct dimensions, and in the same place (in which case you can skip that line).

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 10:59 PM #11

BTW, you can avoid using the "self" parameter on function calls if you declare them a bit differently. Here is an example:


local Instance = {}

function Instance:Show ()
  print (self.name)
end

Instance.name = "nick"

Instance:Show ()  --> prints "nick"

By declaring the Show function as "Instance:Show" (note the colon) it gets an automatic "self" argument, so you don't have to explicitly declare it.

This is a bit more of an object-oriented way of doing it, as it is clearer that "self" isn't just "some argument that happens to be called 'self' " but a genuine reference to the current object by the compiler.

WillFa USA Mon 23 Nov 2009 10:59 PM #12

EDIT: Nevermind, I figured out github... My changes are forked and uploaded, Twisol.

Nick Gammon Australia Forum Administrator Mon 23 Nov 2009 11:44 PM #13

Your code:


    if ok == 30073 then -- eNoSuchWindow
      return false, "no such window"
    elseif ok == 30065 then -- eCannotAddFont
      return false, "unable to add font"
    else

Could be written as:


    if ok == error_code.eNoSuchWindow then 
      return false, "no such window"
    elseif ok == error_code.eCannotAddFont then
      return false, "unable to add font"
    else

This saves coding "magic numbers" into your code.

Twisol USA Tue 24 Nov 2009 02:03 AM #14

Nick Gammon said:

Is this fixed yet? I see on your website a fix entitled "Fixed misspelling of 'cancel' ...".

Which goes on to say '... that would hinder cancelmousedown' specifically.

Nick Gammon said:

Well, you are talking about cancelmouseover, not cancelmousedown, right?

Oops, yes. Minor brain fart which I'll remedy so others don't get confused reading this. (I guess I can't blame you for misunderstanding the commit message!)

Nick Gammon said:

Why do you call WindowCreate in a draw function?

Exactly as WillFa said, it's easier than maintaining a list of changes that have and have not been done, and whether or not to redo ones that belong "behind" the changes on the Z axis.

WillFa said:

OHHHHHHHH... I get it... You're never getting a cancelMouseOver because you're nuking the hotspot in the MouseOver.

Doh. That would explain a lot. This was more of a bug in the workings of the framework than MUSHclient, then. *cough*

Nick Gammon said:

As a workaround either don't re-create the window every time you want to draw in it (after all, then you have to re-add all hotspots, and you have other problems like the one this thread is about, and it is inefficient in that memory is being freed and then reallocated), or if you must have the creation code there, do a check first that the window does not already exist of the correct dimensions, and in the same place (in which case you can skip that line).

It's so much easier to just recreate and redraw, heheh. Hmm, this will require some thought.

Nick Gammon said:

BTW, you can avoid using the "self" parameter on function calls if you declare them a bit differently.

I'm aware, but I've read certain things about being efficient with tables, like declaring things in the table initializer {} rather than after-the-fact. I could probably do something like this, though... and it would be a rather C++-like approach. In fact I'm already doing it with the .new() functions *laughs*.


tbl = {
  foo = nil,
  bar = nil,
}

function tbl:foo()
end

function tbl:bar()
end

I'll probably change the code to do this. I've been juggling design and implementation decisions, though, so I didn't bother with a few things at first (like this).

WillFa said:

My changes are forked and uploaded, Twisol.

Thanks! I'll check them out, I really appreciate the assistance.

Nick Gammon said:

This saves coding "magic numbers" into your code.

I wasn't sure whether I could access error_code and the others without importing constants.lua first (which is really wrapped by XML). Now I see error_code is an entirely separate table, and it seems that I can access it, so thank you for letting me know!

EDIT: Will, your editor is trampling the extra indentation on blank lines. >_> You also might want to look here [1].

[1] http://help.github.com/dealing-with-lineendings/

Nick Gammon Australia Forum Administrator Tue 24 Nov 2009 04:53 AM #15

Quote:

I wasn't sure whether I could access error_code and the others without importing constants.lua first (which is really wrapped by XML). Now I see error_code is an entirely separate table, and it seems that I can access it, so thank you for letting me know!

It is loaded into the script space automagically.

Quote:

It's so much easier to just recreate and redraw ...

"For every problem there is a solution: neat, simple, wrong!"

The ability to recreate windows was really intended for the situation where you needed to load in a font first, and then calculate its size (eg. where its depth is 5 lines deep, depending on the font, and 80 characters wide, depending on the font), and then remake it the right size. Recreating the window between a mouse-down and mouse-up, or mouse-over, and end-mouse-over, was not really part of the design plan.

Twisol USA Tue 24 Nov 2009 05:23 AM #16

Nick Gammon said:

"For every problem there is a solution: neat, simple, wrong!"

The ability to recreate windows was really intended for the situation where you needed to load in a font first, and then calculate its size (eg. where its depth is 5 lines deep, depending on the font, and 80 characters wide, depending on the font), and then remake it the right size. Recreating the window between a mouse-down and mouse-up, or mouse-over, and end-mouse-over, was not really part of the design plan.

Yep, Window.new() creates a dummy window specifically for the purposes you described. I think WillFa and I have worked most of the WindowCreate() issues out now. The only problem is that hotspot management has become more complex, but I'm sure it won't be too much of an issue.

Twisol USA Tue 24 Nov 2009 05:36 AM #17

Nick, would it be possible to add functionality to change a hotspot's mouse handlers after it's been created? It's already there for the drag-related handlers, though I realize that's probably because they were added after the fact.

Nick Gammon Australia Forum Administrator Tue 24 Nov 2009 07:10 PM #18

Just make a generic "stub" routine and then redirect the call into that to where you want. In other words, a handler could look up in your table to see which function (if any) to really call, when it is called.

Twisol USA Tue 24 Nov 2009 07:22 PM #19

That could work, actually, thanks. I'm trying not to litter the user's script-space with anything more than the types he's directly require()ing, but I'll try this and see how it goes.

(Also, I'm trying to save MUSHclient from having to call each and every script callback when it doesn't have to. XD)

Twisol USA Tue 24 Nov 2009 07:41 PM #20

Wait, how do I tell from a stub routine, theoretically used in every handler slot, what it's supposed to be handling? The only parameters are flags and id.

EDIT: Oh, a stub per handler slot. That seems like a lot of bloat, and a fair amount of litter in the script space. If Drag/Move/ReleaseHandler can be changed, why can't the others? It seems simple to do, from the cursory glance I gave to the implementations of CMUSHclientDoc::WindowDragHandler() and CMiniWindow::DragHandler().

Nick Gammon Australia Forum Administrator Tue 24 Nov 2009 08:24 PM #21

Why do you want to do this exactly? You already have cell.hotspot.mouseup etc. for every hotspot, right? All I am saying is, you create those functions on window creation, even if the function contents are replaced later on.

Twisol USA Tue 24 Nov 2009 08:58 PM #22

Nick Gammon said:

Why do you want to do this exactly? You already have cell.hotspot.mouseup etc. for every hotspot, right? All I am saying is, you create those functions on window creation, even if the function contents are replaced later on.

Because the widget framework can't control what the user does with the hotspot callbacks. If cell.hotspot.mouseup is nil, there's no callback there to use. Creating the functions unconditionally on window creation, from the framework code, litters the user's script space, and restricts how it can be used. If the dummy-functions are placed in _G, each window would need hopefully-unique names for its handlers. If the dummy-functions are placed in the cell or the grid tables, then the cell/grid must be global to the script space. But if you can change the hotspot callbacks any time you want, you have more control over how and when they're called, even up to the point of swapping out different handlers depending on the current state of your plugin. All without specifically recreating the hotspots and potentially interrupting their own state.

Allowing the callbacks to be set after the fact also means that MUSHclient wouldn't have to bother with calling a dummy callback that's only there to fill space, if it's never going to be used in the specific application at hand. And again, it allows the widget framework to keep the script space clean, and also keep its design clean.

I don't think this is a feature that would only benefit the widget framework, either. I'm simply using hotspots enough to have noticed what appears to me to be a drawback.

WillFa USA Tue 24 Nov 2009 09:35 PM #23

Hotspots only work in plugins, which are fairly static once they're coded. Placing the burden on the plugin author not to screw up his own variable and function names doesn't seem like a stretch.

Twisol USA Tue 24 Nov 2009 09:46 PM #24

No, however if I have to give each handler a unique global name, then the plugin owner will have to set each global handler. Even if they'll all refer to the same function, you'd still have to set each generated name to point to it. And since you could have an arbitrary amount of windows/hotspots, that could be very... very annoying.

WillFa USA Tue 24 Nov 2009 09:58 PM #25

what's hard about?

cell.hotspot.mouseover or ""

If a plugin author doesn't want em, he don't get em. If he sets hotspot somewhere up the inheritance cascade, so be it, that works.
The only real limitation is that an author would need to set the 5 values in the hotspot table (or tables) before Draw is called and a hotspot is created, or Resize the window after declaring a new handler so all hotspots are nuked and recreated.

In 99% of cases, the implemented design is fine. In the 1% fringe case, it's simple enough to fix in the case of hotspots.

...
Plugin Callbacks in general though can be a pain in the butt. You can dynamically create a function for OnPluginWorldResized (for example) but if its not there when compiled, MC doesn't hook into it.

Twisol USA Tue 24 Nov 2009 10:30 PM #26

The problem with that particular approach is that it doesn't allow you to unhook functions or add new ones. The approach I assume Nick was speaking of, however, was creating a blank 'default' handler, which the user could override. I enumerated the drawbacks I could see in my previous couple posts.

I could solve this on the scripting side of things, but it would come with drawbacks no matter how you look at it. I am still wondering why we're so against allowing the handlers to be changed after the fact, when the drag-related handlers already support it, and it appears quite simple to do. It won't add any complexity that's not already there, and would simplify things as well.

EDIT:

WillFa said:

In 99% of cases, the implemented design is fine.

The design -is- fine. Hotspots work beatifully. I just want to be able to set certain aspects after they've already been created. That doesn't change the design, just how you utilize it.

Nick Gammon Australia Forum Administrator Thu 26 Nov 2009 08:21 PM #27

My suggestion here is, since you are doing a framework anyway, to not store in your table that you already have, the *names* of the functions you want called (as they have to be in global namespace somewhere) but the functions themselves.

That was the approach I took in the movewindow.lua. When doing the AddHotspot, each action was given a callback in the unique table for that window (there has to be *one* thing in global space) eg.


mw_UNIQUE_NAME_movewindow_info.mousedown
mw_UNIQUE_NAME_movewindow_info.cancelmousedown
mw_UNIQUE_NAME_movewindow_info.mouseup
mw_UNIQUE_NAME_movewindow_info.mouseover
mw_UNIQUE_NAME_movewindow_info.cancelmouseover

This uses one thing in the global address space: mw_UNIQUE_NAME_movewindow_info.

Now in those table entries are not the names of what functions you want to call, they are the functions themselves. This means you don't have to invent lots of extra names for the functions.

Also, as in movewindow.lua the functions are created by a function-generating function which creates the functions with an upvalue, which is the table with all the other info in it, so the functions are self-contained.

Twisol USA Fri 27 Nov 2009 06:39 AM #28

Thanks! I've adapted that approach slightly, so the strings look like this instead:


MWidget.Window.hotspot_handlers.mouseover
MWidget.Window.hotspot_handlers.cancelmouseover
MWidget.Window.hotspot_handlers.mousedown
MWidget.Window.hotspot_handlers.cancelmousedown
MWidget.Window.hotspot_handlers.mouseup
MWidget.Window.hotspot_handlers.dragmove
MWidget.Window.hotspot_handlers.dragrelease

Each hotspot ID looks something like this (taking an example from a CharacterGrid hotspot):


w1_37859615f6c4f74b6872f8e6-h(1,1)

Everything before the -h is the hotspot's window's ID, so the constant handlers mentioned above can figure out which window table to use. It then goes into that table's list of hotspots, grabs the specific hotspot's details, and executes its callback (if it exists).

I think I'm content now *laughs*. It would be nice if I could change a hotspot's cursor after-the-fact, though, like the tooltip can be.