GNOME Bugzilla – Bug 744679
undo gives wrong status message in some games
Last modified: 2019-10-16 18:17:29 UTC
Created attachment 297044 [details] [review] "" After upgrading from Ubuntu 12.4 to 14.4 (which has aisleriot 3.10.2), I found two annoying regressions in my favorite waste of time, a.k.a. Solitaire: - 'undo' no longer updates the status message (so on Spider, for example, when undoing or re-doing past a deal from stock, the 'stock remaining' count isn't updated) - dragging cards to the window edge makes a complete mess out of them Both still exist in the newest sources (3.15.0, as far as I could tell), so I set out to try and debug them. My solutions are attached. They are both very simple - though I have to admit that they might not be up to standard. That's as good as I can do, given that both Scheme and GTK/GDK are technologies that I have never worked with before. More technical detail and possible alternate solution for the missing give-status-message on undo/redo: (A) Scheme/guile and the 'undo' regresion: the change to modules caused the name spaces of the API (sol.scm, which became api.scm) and the games to become separate, therefore the call from api.scm to give-status-message always reaches the definition found in api.scm itself, even if there is an alternate defined in the game's .scm file. I made api.scm "peek" into the game's main module and try to call its give-status-message if there is one there. Admittedly the Guile construct that I used for that is rather messy, but I did not find a better way. I also found another way to achieve the same effect, without using clunky module lookups - but it requires modifying every game that has 'give-status-message': - replace (define (give-status-message) ...) in the games with (set! give-status-message (lambda () ...). This makes it update the definition in api.scm. To make it actually work, the (define-public (give-status-message) ...) construct in api.scm also has to be modified to use explicit lambda: (define-public give-status-message (lambda () ...). Even though logically the shortcut define and the lambda construct should be the same, it appears that they are not - possibly, when the (define (function) ) syntax is used, Guile "inlines" the call to give-status-message within api.scm, making the override from the games ineffective. (B) the "smearing cards" problem: After much poking around to try to spot the bug, I added gdk_window_set_debug_updates(1) on drag_begin() - and to my horror, GDK clearly showed that the cards window is actually NEVER painted. The current code appears to work (somewhat) simply because the update of the card stack to remove the dragged cards is done AFTER a non-transparent child window (the 'moving_cards_window') is plopped on top of it (all of the exercise done drag_begin() to get the card images and carefully draw them on a Cairo surface that becomes the windows background has only one effect - to mark the exact area where the cards are as being non-transparent - the generated background pattern is never actually used :P ). Not being a great expert at GDK, I used the first solution that I could find: call begin_paint and immediately end_paint, which did exactly what was wanted - to draw the Cairo background associated with the window.
Created attachment 297046 [details] [review] ""
The card drawing problem is tracked in bug 702951.
The patch for the guile issue seems to have got lost; here it is from the ML post: -(define-public (give-status-message) - #t) +(define (give-status-message) + (if (module-variable (resolve-module '(guile-user)) 'give-status-message) + ( (@@ (guile-user) give-status-message) ) + (#t)) ) @Vincent: does that look reasonable to you, or can you think of a cleaner way to fix this?
I was not aware we called give-status-message from outside game-specific code. I thought it was just a convention. Why do we need to do this? It seems cleaner to me to stop giving it special treatment and call game-over more often.
I see, Spider doesn't call it from game-over. Probably because we were giving that function name special treatment while sort-of and sort-of-not relying on it. Ugh.
Based on a quick scan, only the following games define give-status-message and do not call it from game-over: first-law, kansas, lady-jane, spider, and straight-up These games seem to be working on the assumption that they only need to update the status message when something happens that would cause it to change. Spider and First Law only have the stock size in the status bar, so they only update it when dealing. I think the expectation was that the status bar message was state that would be saved and restored by undo/redo, which would fix the bug in a way that's least surprising to people like me who mostly only deal with individual game code. And it would mean give-status-message doesn't need any special treatment. I don't know how hard that would be to implement. Eliminator calls give-status-message but does not define it or import it from anywhere, and there may be others that do something similar, so we'd have to keep it defined in api.scm.
@Bhavik: thanks for opening this bug for me (someone on the ML told me to come here and post the bugs - I see you beat me to it :) I agree that fixing the individual affected games might be cleaner than having back-and-forth calls between the api and the games. I did consider this when looking for a fix, but I did not find a useful event that invokes code in the game when 'undo' or 'redo' is clicked - as far as I can tell, 'undo' ends up calling 'dealable?' and redo calls 'game-over' - but putting the give-status-message call in 'dealable?' didn't seem like a very logical thing to do. In any case, when considering the fix for this, please note that I intend to push the Ubuntu team to have the fix applied in 14.4, so it would be helpful if the bug fix is compatible with Aisleriot 3.10 (if it involves minor changes only in .scm files, it should be compatible - they haven't changed very much between 3.10 and 3.15).
Created attachment 374241 [details] [review] Save and restore status messages (In reply to Vincent Povirk from comment #6) > I think the expectation was that the status bar message was state that would > be saved and restored by undo/redo, which would fix the bug in a way that's > least surprising to people like me who mostly only deal with individual game > code. And it would mean give-status-message doesn't need any special > treatment. > > I don't know how hard that would be to implement. I like this and I think it would be pretty easy to do. I'll attach a patch. (In reply to Vincent Povirk from comment #6) > Eliminator calls give-status-message but does not define it or import it > from anywhere, and there may be others that do something similar, so we'd > have to keep it defined in api.scm. I removed the call to 'give-status-message' from Eliminator in an earlier commit. None of the games are using the 'give-status-message' defined in api.scm anymore so I removed it in the attached patch. (In reply to Christian Persch from comment #3) > -(define-public (give-status-message) > - #t) > +(define (give-status-message) > + (if (module-variable (resolve-module '(guile-user)) 'give-status-message) > + ( (@@ (guile-user) give-status-message) ) > + (#t)) ) One problem with this is that every game should define 'give-status-message' for this to work. Otherwise, suppose that game A defines it and game B does not. Then if you load game A to play it, and load game B after that, then this would still call 'give-status-message' from game A and not the one in api.scm, causing similar problems as in bug 779149.
Thanks for the patch! Looks fine, so I've committed it. I think with that, this bug can be closed FIXED, right?
Thanks! Yes, and this similar issue on gitlab too: https://gitlab.gnome.org/GNOME/aisleriot/issues/1