Better error handling

[pablomayobre]

Currently ScreenManager doesn't check:

• If the screens are tables when calling ScreenManager.init. So it will error when trying to index screen.new()
• If the returned value from screen.new() is a table. So new() could return a string (for example) and later on when trying to index it in search for the callbacks you would get an error trying to index a string
• If the callback is defined. So it will error when trying to index a callback which wasn't defined in the screen
• If the callback is actually a function. So if for example screen.directorydropped was a string ScreenManager.directorydropped would try to call a string resulting in an error

I know that most of this issues can be fixed by using the suggested Screen.lua as a base class, but it would be great if ScreenManager errored when it found one of this akward cases (which are of course not the libraries fault but the users fault)

I consider that the following would be the best way to handle each case:

• In the case of new() not returning a table it should probably error
• In ScreenManager.init if the nscreens wasn't a table containing tables that have a .new() function then it should error
• If the callback is not defined it should be ignored
• If the callback is defined but not a function then it should be ignored

[rm-code]

If the callback is not defined it should be ignored
If the callback is defined but not a function then it should be ignored

I'm not a fan of silently ignoring errors. If the user calls a callback that doesn't exist I expect the program to fail (which Lua does in these cases by default). But I also don't want to burden the user with the overhead of checking the validity of callbacks each time they are called.

In ScreenManager.init if the nscreens wasn't a table containing tables that have a .new() function then it should error

This is something we could do, but where do we stop taking the user by the hand? I mean checking if it returns a table is a pretty weak test. After all we don't know if the returned table is valid.

So I'd say these are all checks that should be done on the user's side rather than in the library.

[pablomayobre]

I proposed that we should ignore errors in callback because you may not want to define some event for a state, for example if you don't need the filedropped event in your game state then you can leave it blank, and it will be just fine.

Not registering the callback may be an alternative but you may need that event for other state (say the main menu)

Currently this is fixed by putting stub functions to all the callbacks in the Screen base class. What I'm suggesting would avoid the need to define all those fields by default

About the hand holding and where do we want to stop. I suggest that the library should check if nscreens is a table. Then validateScreen can check if the screen is a table with a new function and otherwise consider it an invalid screen.

[pablomayobre]

Closed by accident

[pablomayobre]

Something like this should be enough for the screens:

local function validateScreen( screen )
    if not screens[screen] or type( screens[screen] ) ~= "table" or type( screens[screen].new ) ~= "function" then
        local str = "{"

        for i, _ in pairs( screens ) do
            str = str .. i .. ', '
        end

        str = str:sub( 1, -3 ) .. "}"

        error( '"' .. tostring( screen ) .. '" is not a valid screen. You will have to add a new one to your screen list or use one of the existing screens: ' .. str, 3 )
    end
end

function ScreenManager.init( nscreens, screen, ... )
    if type( nscreens ) ~= "table" then
        error( "The first argument to ScreenManager.init should be a table containing all the available screens", 2 )
    end

    stack = {}
    screens = nscreens

    validateScreen( screen )

    ScreenManager.switch( screen, ... )
    ScreenManager.performChanges()
end

The callbacks would be a little bit harder since there would be some code repetition over there

[rm-code]

Ok, but then validateScreen should be called in both push and switch rather than init?

The callbacks would be a little bit harder since there would be some code repetition over there

Yeah and I don't like the overhead it would create. But if we ignore faulty callbacks like you suggested above these are superfluous anyway.

I proposed that we should ignore errors in callback because you may not want to define some event for a state, for example if you don't need the filedropped event in your game state then you can leave it blank, and it will be just fine.

We could do that and get rid of Screen.lua

[pablomayobre]

Ok, but then validateScreen should be called in both push and switch rather than init?

validateScreen is called in both push and switch (and also in init in order to give a proper error stack trace)

Performing the proposed change would mean that the nscreens table can hold other stuff besides screens but if you try to set the state (either with push, switch or in init) to something that is not a valid screen with a new() function, it will error.

We could do that and get rid of Screen.lua

Yes this is a good goal.

Yeah and I don't like the overhead it would create. But if we ignore faulty callbacks like you suggested above these are superfluous anyway.

I don't think that the overhead is big, it's just an if type(callback) == "function" then every time you would call a callback

Consider that not all callbacks are triggered each frame, so lets say 6 callbacks in a frame, and you may have a stack height of 10, the worst case would be that the callbacks all loop through the whole stack, that would be 60 times in a frame you would be performing an extra check and a function call which are both JIT compiled

[rm-code]

If we get rid of Screen.lua we also need checks for the init and set / isActive functions or make them optional.

[pablomayobre]

Totally right! there is also close... Committed to my propagate branch

There is no need to check for isActive since it's not used internally by ScreenManager

There is however the problem of screen.new() not returning a table and trying to index it... but the error will probably show up when trying calling ScreenManager.performChanges which is not quite natural

So I think that it should be noted somewhere with BIG LETTERS AND IN BOLD that screens are tables which have a new function which returns a table with the callbacks it needs. Returning a non-table value from that new function (including returning nil) will result in an internal error of the type Trying to index an X value where X is the type of the value returned by new