2007-12-19

Scrolling Panels

The ScrollingPanel gadget now works. It can scroll its content in any direction using the DMA hardware. Scrolling vertically was easy to implement, and it works in the same way as the TextViewer class:

  • Work out how far we want to scroll in pixels.
  • Copy all visible rows up or down that number of pixels.
  • Fill the region that has scrolled into view by calling the draw(Rect) routine.

The major difference between the ScrollingPanel and the TextViewer is that the ScrollingPanel scrolls individual visible regions. It’s got clipping built-in, whereas the TextViewer still doesn’t have clipping because the iteratively-developed code is a godawful mess.

Scrolling horizontally was more complicated. Back when I was playing with solid-window dragging, I decided that the only way to perform horizontal scrolling using the DMA hardware was to:

  • Work out how far we want to scroll in pixels.
  • Loop through all visible rows.
  • Copy chunks of data the width of the scroll distance left or right along the row.
  • Fill the new regions by calling draw(Rect).

You can’t just copy 10 pixels from (x = 1) to (x = 5) because they’re copied as individual u16s. By the time you’ve copied half of the pixels and reached (x = 10), you’ve overwritten the second-half of the data you wanted to copy.

I implemented the above routine and ran into a problem. If we try to scroll an area of 10x10 pixels by one pixel, we have to copy the whole area pixel by pixel. This results in 100 copy operations. I was aware of the issue before I started, but decided it was worth a go anyway. It wasn’t too bad - it was certainly fast enough under emulation.

However, the obvious solution finally presented itself. If the routine creates an off-screen array of u16s with a length equal to the width of a row, I can just copy the whole row to the array and then copy it back to its new horizontal position. One extra copy routine and a memory allocation is a lot faster than 100 copy operations and associated calculations. It would probably even be feasible to have solid window dragging using that technique.

I’m not certain at this stage how well the ScrollingPanel will function as a gadget container. It currently has a method to adjust the positions of any child gadgets when the panel is scrolled, but it does not redraw its children. The problem is that we only want a small portion of a gadget to redraw, but the existing Gadget::draw(rect) function assumes that the rectangle parameter represents a valid, visible region, and will draw it without any further checks. I’d need to either:

  • Implement a new draw routine that will accept a clipping rectangle, but clip that rectangle to the gadget’s internal region cache before drawing, or;
  • Just call draw() on all children and allow them to completely redraw themselves.

The first option is trickier than you might expect, to the point that it really isn’t justified. The second option is easy, but will result in flickering child gadgets as the panel scrolls. On the positive side, I think I should be able to modify the MultiLineTextBox so that inherits from the ScrollingPanel and thus immediately equip it with omni-directional scrolling functionality.

In order for scrollbars to work, the ScrollingPanel needs to keep a note of the lowest pixel drawn within it (so that we know how tall the draw space is). Must remember to do that. Or possibly some other mechanism that achieves a similar result.

Comments

Jeff on 2007-12-19 at 21:38 said:

I downloaded the current sources, and was a little confused.

It seems that the scrolling panel automatically handles scrolling by stylus, but relies on its owner to do scrolling by keypad. Wouldn’t it be more of a natural fit for the scroller to take focus and handle the keys as well?

Or is the expectation that this is an enabling technology that other gadgets get built on top of, and that the stylus dragging will move out of the panel and into those other gadgets?

(I’m trying to see if its going to change the way my List gadget works…)

ant on 2007-12-20 at 00:30 said:

Scrolling by keypad is only in main.cpp for testing, but if you think it’s useful to have by default I can bundle it into the class.

Jeff on 2007-12-20 at 02:42 said:

Well, I think you need to decide one way or the other as to whether the scrolling panel is going to interpret user input. Personally, I think it should. My List does.

The containing window can still intercept the keys prior to them being delivered, can’t it? Or does that happen in Woopsi:: ?

ant on 2007-12-20 at 09:32 said:

Key presses are sent from Woopsi down the active gadget chain. Each gadget has an _activeGadget pointer which points at that gadget’s active child. To get at the gadget that currently has focus, I recurse down the tree until I hit a gadget whose _activeGadget pointer is NULL - that means the current gadget has focus.

Every gadget along the way is allowed to process the keypad events, so they can interpret them if necessary (contrast this with a stylus click, which is solely processed by the gadget that was actually clicked). This means that the window could prevent the ScrollingPanel from receiving keypresses, but only if the developer had subclassed the Window class and overridden the keyPress() and keyRelease() methods in order to change their behaviour.

Jeff on 2007-12-20 at 11:13 said:

Yes, thats what I was expecting. The Window would eat the key strokes that it wanted to implement differently. Is it possible for the Window to intercept the ‘stylus’ events as well, in the same manner?

(Not sure if I’d want to, just curious - I think the natural fit is still for the gadgets to process “physical” events and pass messages out to their container for the corresponding “logical” events, rather than dictating what the actual logical consequences must be.

Though, did the Screen decorations clear this up, or do they still insist that “if you click a depth gadget, it knows that it has to invoke the ‘change depth’ in its enclosing screen, rather than just sending an activated message to the screen, and letting the screen decide (based on gadget->getRefcon()) which button it was, and therefore what action to take???

Sadly I’m away from my source code at the moment - packing to fly tomorrow. Holidays in the sun, yippee…

ant on 2007-12-20 at 13:21 said:

Gadgets can override any of the event functions. In fact, you have to - the Gadget class doesn’t send clicks, releases, key presses, etc to children by default.

I’ve been debating whether the Gadget class should do this for ages.

Arguments against:

  • If a gadget automatically sent clicks to children, any gadgets that shouldn’t send those clicks would need to have those functions overridden.
  • End-point gadgets would have extra code for no real reason.
  • If the default routine didn’t work as required it would have to be overridden anyway.
  • Having standard click() and release() functions that process only the current gadget is useful for firing logical events in a consistent fashion.

Arguments for:

  • Gadgets used as containers (screen, window, scrolling panel) need to implement all of the physical event routines in order to pass events to their children; most of that code is duplicated.
  • All gadgets will function as containers out-of-the-box (though their utility and behaviour as containers may not match expectations).

There’s probably a third option available involving the creation of new methods within the Gadget class specifically designed to handle child event routing. I need to implement more container gadgets to really get a good picture of how it will all hang together.

The screen buttons still insist on calling their parent’s depth swap or flip screen functions. Checking by refcon is an interesting idea, though. One to look into after the ScrollingPanel stuff is all finished.

Anyway, yep - subclasses of Window can implement different click() routines and miss out the loop that tries to route the event through the child classes. That will send the click straight to the window itself.

Jeff on 2007-12-20 at 20:23 said:

“Gadgets that shouldn’t send the clicks …”

Would these things have children anyway? That seems like a fairly uncommon case, especially if you remove decorations from the picture. Its an interesting optimisation to consider whether the container should look at whether the child is disabled before sending the click on. I think that that is how Windows works, rather than what you currently have where the gadget always recieves the click and has to say “I’ll ignore this because I know I’m disabled”. If the enabled() method was inline, there would be no performance hit for the container to do it, and no violation of object privacy.

“End-point gadgets would have extra code for no reason”

Being realistic, they wouldn’t “have” extra code - they’d share the code they have inherited. And if its looking at the _gadgets.size() they won’t be executing any unneeded loops, other than the first integer comparison, so its not that expensive.

I have to say, there’ve been times when I’ve been looking through the code and wondering “why is it bothering to call back to Gadget::click() at this point” - I think I’ve commented before on the code in Window::click() variable that uses _clickedGadget as a control variable and requires the child in the loop to set it for the parent, even though the loop could just look at the child having returned true.

The idea of helper methods for “common chores” is definitely a good one - one thing I was wondering about your recent DMA scrolling code was whether it was available as a general GraphicPort capability.

In MacOS Classic, there was a function called ScrollRect(), and I’m pretty sure that the same or similiar thing exists on Windows. It just takes an enclosing rectangle, and an amount to shift it by, and it handles all the clipping, optimisation, etc. (I believe that as a side effect, it adjusts the ‘update required’ region to let the window manager know about the ‘newly exposed’ pixels - now, Woopsi doesn’t have such a concept at this point, and its probably overkill to consider it).

But having the ability for any Gadget to exploit the scrolling capability would be very useful. Imagine a rolling digital odometer where the bottom digit is constantly scrolling up while the tens and hundreds only scroll when they need to.

all of the physical event routines in order to pass events to their children; most of that code is duplicated.”

This is definitely a good reason for factoring out the convenience methods, and it helps to identify the subtle differences between things that may be by design, and may just be cut/paste bugs.

One other thing that I’m still wondering about is the individual methods for handling raised events. Whilst I recognise that distinct methods for raiseXXX() are critical (since they guarantee that you get passed the correct arguments, and populate the EventArgs object correctly), the handleXXX() methods all get passed exactly the same argument, and are easily confused. They bloat the Gadget’s vtable with lots of extra handlers that most of them really don’t need. I would be changing that across to a single handleEvent() and any implementors can then put a switch() inside it based on the actual event type (which would be one extra field).

If you then reserve types that are >0x1000 for “application” messages, then you use the same mechanism for communicating throughout the app. For example, my MenuScreen currently does this:


// define local button refcons
enum {
        BROWSE_BUTTON = 1000,
        POWER_BUTTON
};
...
        b = new Button(r.x, r.y, 64, 16, "Browse");
        b->setRefcon(BROWSE_BUTTON);
        b->setEventHandler(this);
        addGadget(b);

        b = new Button(r.x+100, r.y, 64, 16, "Power");
        b->setRefcon(POWER_BUTTON);
        b->setEventHandler(this);
        addGadget(b);
...

void MenuScreen::handleRelease(EventArgs e)
{
        // ignore it if the stylus is not in the button still
        if (!e.gadget->checkCollision(e.eventX,e.eventY)) return;

        switch (e.gadget->getRefcon()) {
        case BROWSE_BUTTON:
                _theApp->FileBrowser();
                break;
        case POWER_BUTTON:
                IPC->aux |= BIT(6);
                break;

 }

but that last part could just as easily be:


void MenuScreen::handleRelease(EventArgs e)
{
        // ignore it if the stylus is not in the button still
        if (!e.gadget->checkCollision(e.eventX,e.eventY)) return;

        switch (e.gadget->getRefcon()) {
        case BROWSE_BUTTON:
                raiseApplicationEvent(APPEVENT_START_BROWSER);
                break;
        case POWER_BUTTON:
                raiseApplicationEvent(APPEVENT_POWER_OFF);
                break;
 }

This has the added bonus that any container above this one can intercept the raise, put up a “Save First?” dialog, then just raise it again. It does mean that any gadget needs to implement its handleEvent() as


::handleEvent(EventArgs &e)  // should be a reference, currently isn't
{
  switch (e.type) {
  default:
    // we don't handle this one, propagate to parent
    return getParent()->handleEvent(e);
  }
}

This sort of “higher-order” message approach solves your “switch screen depth” issue in that ANY gadget then has a neutral way to request that the screens switch around. My development code is a mess at the moment because I can’t see the debug screen after its been pushed to the back, because switching it to the TOP also switches a non-decorated splashscreen in front of my buttons. My own fault, but it illustrated for me the need to be able to override the depth/screen switching algorithms. Message switching would make that easier for me, currently, I’d have to override the ‘do the switch’ method (because of the way the screen depth buttons work) rather than just the ‘ask for the switch to be done’ method…

Which is also another way of saying that factoring out the ‘useful sub-methods’ is pretty much always going to be a good thing

Jeff on 2007-12-20 at 20:24 said:

(I think there’s another essay in the spam filter)

Jeff on 2007-12-20 at 20:34 said:

In reply to my message that got spammed away, but hopefully will come back…

The one issue I see with adding application messages to request stuff is that it prevents the dead-code remover in the linker from taking functions out if they are not called. A big switch statement that references a function will definitely lock it into the binary, since the compiler can’t tell that that particular switch path will not be taken. However, in most of the existing Woopsi classes, that wouldn’t make a difference - but I wouldn’t start dumping more and more code into the base classes just in case it got used. Rather I’d have inline (non-virtual) helpers in the base class, and rely on subclasses to switch to them if they want to. That way, those who want the extra functionality get it, and those who don’t want it, don’t get it.

ant on 2007-12-20 at 23:59 said:

Interesting stuff - I’ll have to read through it more carefully when it’s not midnight and then have a ponder!

ant on 2007-12-21 at 10:40 said:

OK, this looks good. Summarise to make sure I’ve understood everything:

  • Add an “event type” variable to the EventArgs struct that will hold a u32.
  • Remove all “handleXXX()” methods from the EventHandler and replace with a single “handleEvent()” function that switches the new “event type” value to determine what to do with the event.
  • Add a “raiseApplicationEvent(type)” function to the Gadget class.

The only part I’m not sure about is having parents handle child events. This is quite a big change and I’d need to think about how it affects everything first.

I’m not convinced that sending messages through an event system is really the right way to do it; I’d be more tempted to implement a dedicated messaging system. Gadgets would have a “processMessage(Message msg)” function that switches the msg.type parameter and decides what to do with it. In the case of the flip screen button, it’d do this:


Message msg;
msg.gadget = this;
msg.type = MESSAGE_FLIP_SCREENS;
_parent->processMessage(msg);

Instead of this:


((Screen*)_parent)->flipScreen();

The child tells the parent what it wants to happen, and the parent, regardless of what it is, can then do what it likes with that message - escalate it up the tree, process it, whatever.

In the case of the click() function, I’d do this:


Message msg;
msg.gadget = this;
msg.type = MESSAGE_UPDATE_CLICKED_POINTER;
_parent->processMessage(msg);

The parent would do this:


bool processMessage(Message msg) {
   switch (msg.type) {
        case MESSAGE_UPDATE_CLICKED_POINTER:
            _clickedGadget = msg.gadget;
            return true;
        default:
            return false;
    }
}

The problem with having the handleEvent() function embedded in the gadget is that it becomes much more difficult to work with the system unless you’re subclassing. Obviously not a problem for you, but much more of an issue for the guys who have trouble with programming basics.

I still see events as a way of notifying external code that something has happened within Woopsi, and don’t think that gadgets should use events internally to communicate. Similarly, I wouldn’t use the proposed message system as a way to communicate externally.

ant on 2007-12-21 at 10:55 said:

This would still give you the ability to have your gadgets handle shutdown (etc) events, though - if you’re subclassing anyway, you can have your gadgets inherit from the EventHandler and implement handleEvent(). From there, it’s up to you how those events get passed around.

I think handleEvent() from the EventHandler, coupled with raiseApplicationEvent() (which I’d be tempted to change to the more generic “raiseEvent(type)”), gives you a method for achieving what you want.

ant on 2007-12-21 at 11:48 said:

The following things are in there:

  • EventArgs passed around as a const ref.
  • EventHandler events reduced to handleEvent().
  • Added EventType enum to EventHandler.

Are application events really the responsibility of Woopsi? I think it should be down to the subclasser to implement that side of things. If I start prescribing the way application events are passed around I’m dictating the way that applications are built, and I’m back to task managers and Task classes again.

Jeff on 2007-12-22 at 07:12 said:

Lots to comment on. You definitely have the gist of what I was talking about, though the need for distinct “Event” vs “Message” escapes me. They are still just “a logical event/message needs to be handled”. When its a “physical event” like the main loop detecting that the stylus is now pressed, or that a key has been released, thats being passed down the hierarchy by dedicated methods, which I think is a good thing.

When one of those physical events has some kick-on consequence that might affect the rest of the application, it is propagated back UP the hierarchy by Events/Messages.

The benefit of the raiseXXX() methods is that they ensure you provide all valid information for that particular event type. A ‘raiseApplicationEvent()’ would really just raise a pre-existing EventArgs object - it doesn’t need to be an applicationEvent - you could use it to raise a click, if you really wanted to.

The click one is a really bad example, I think, because its using logical messages to implement an internal gate - the low level gadget should be saying Woopsi::setFocus(this) or some such - letting the application know that this gadget wants the focus because thats what it wants - a gadget should not want the application to “please update the control variable to point to me”

Your parent example looks incorrect. In the case of not handling the message, it should be propagating it up to its parent. Yes, you’ll need to test for null. etc. Coding exercise for a student ;-)

As to events being the responsibility of ‘Woopsi’, I don’t quite know what you are saying? If you mean the top-level object, then yes, it needs to at least catch them and return OK. If you mean the framework in general, then I think yes, the framework should provide the mechanism in a simple consistent way. Remember, you didn’t want people subclassing Woopsi originally.

You are already prescribing the manner used to propagate around control messages - I can’t implement a different screen-swapping mechanism in my application without hacking on your implementation methods; thats infinitely worse than me tapping into a clean messaging system. I don’t need to use messaging for MY application logic, but by using it to interact with yours, I future-proof myself against you changing methods (names, constifying, adding argments, etc) all of which silently screw up sub-class-method-overrides and can be a pig to find.

Jeff on 2007-12-22 at 07:18 said:

(spam again?)

Re: “The only part I’m not sure about is having parents handle child events. This is quite a big change and I’d need to think about how it affects everything first”

I’m not sure what your concern is. What I’ve proposed is an extensible mechanism for children to pass requests to parents - you don’t currently use Events to communicate with children, you call them directly by context. ie, the parents click() method knows to call its childrens click() methods directly.

Yes, you could glom all those things back into individual events, but I think the type safety thats embedded in those hard-coded methods more than makes up for any theoretical flexibility that going to a Win32 like ‘messages for everything’ approach.

So, to repeat. As it stands at the moment, I think there is no difference between Event and Message, one is just a linguistic specialisation of the other. You don’t currently use Events for PHYSICAL event processing, only for communicating from child back to parent, as far as I can tell, and I think thats a good thing.

Personally, I’d prefer Message to Event because Event does carry the overtone of ‘something happened’ which is also the case for all “Message”s

ant.simianzombie.com » Blog Archive » Refactoring Events on 2009-04-01 at 21:48 said:

[...] followed this pattern, as described back in September 2007, but swapped to the single method on Jeff’s advice. As Woopsi has grown, though, this approach has led to some fantastically bloated and unintuitive [...]