[Bug r3456] Load/Save Game Frame

[knightly]

@Prissi

A very curious bug

When loading a file :
1) click on a file entry, drag and release on another file entry; repeat again#  ==>  a message showing "Cannot load saved game!" is displayed
2) click on a file entry, drag and release on another file entry; then single click (just click) on a file entry# ==> game is loaded, but both "Game successfully loaded" and "Cannot load saved game!" messages are displayed

When saving a file :
1) click on a file entry, drag and release on another file entry; repeat again#  ==>  game is saved, but it is saved in a blank file entry (underlying file is ".sve" with nothing before the dot and file extension)
2) click on a file entry, drag and release on another file entry; then single click (just click) on a file entry#  ==>  game is saved, but saved twice : first time on overwriting the last file entry clicked, second time on creating/overwriting a blank file entry (i.e. ".sve" as described above)

# Note : no need to be the same file(s) involved in the previous click-drag-release

True, no sensible player will attempt such meaningless actions on the load/save frame . But these problems revealed some hidden problems with the code :

1) in gui_container_t::infowin_event() :

   // handle unfocus/next focus stuff
   if(  new_focus!=komp_focus  ) {
      gui_komponente_t *old_focus = komp_focus;
      komp_focus = new_focus;
      if(  old_focus  ) {
         // release focus
         event_t ev2 = *ev;
         translate_event(&ev2, -old_focus->get_pos().x, -old_focus->get_pos().y);
         ev2.ev_class = INFOWIN;
         ev2.ev_code = WIN_UNTOP;
         old_focus->infowin_event(&ev2);
      }
   }

the above code will try to create a WIN_UNTOP event and pass it to the component which has just lost focus.

2) then in the last portion of gui_textinput_t::infowin_event() :

   else if(  ev->ev_class == INFOWIN  &&  ev->ev_code == WIN_UNTOP  ) {
      call_listeners((long)0);
      return true;
   }
   return false;
}

a WIN_UNTOP event will trigger calling of registered listeners

3) finally in savegame_frame_t::action_triggered() :

   if(komp == &input || komp == &savebutton) {
      // Save/Load Button or Enter-Key pressed
      //---------------------------------------

      if (strstr(ibuf,"net:")==ibuf) {
         tstrncpy(buf,ibuf,lengthof(buf));
      } else {
         tstrncpy(buf, SAVE_PATH_X, lengthof(buf));
         strcat(buf, ibuf);
         strcat(buf, suffix);
      }

      action(buf);

      destroy_win(this);      //29-Oct-2001         Markus Weber    Close window

   }

without knowledge of the original event that triggered this action, the code only checks for object identity (komp == &input returns true), hence the code for loading/saving is triggered, neither because of a click on the load/save button, nor because of pressing ENTER when the text input has focus.

As to the reason why the first click-drag-release does not trigger this :

         // Hajo: infowin_event() can delete the component
         // -> thus we need to ask first
         gui_komponente_t *focus = komp->get_focus();

         swallowed = komp->infowin_event(&ev2);

         // set focus for komponente, if komponente allows focus
         if(  focus  &&  IS_LEFTRELEASE(ev)  &&  komp->getroffen(ev->cx, ev->cy)  ) {
            /* the focus swallow all following events;
             * due to the activation action
             */
            new_focus = focus;
         }

focused component is fetched before infowin_event() is called on the component; hence for the first time a focused component is requested from the inner scrollpane, null is returned (as initially there is no focused component inside that pane) and the text input isn't displaced as the focused component of the load/save frame; but for the second time, a focused component is fetched from the inner pane, thus displacing the text input as the focused component of the load/save frame, which in turn triggers the sequence of actions above.

Honestly I don't know enough of the focus mechanism to fix this. I don't know why WIN_UNTOP event should trigger registered listeners in the text input component. And I don't understand why focused component is fetched before calling infowin_event() as quoted above -- if a component is deleted by infowin_event(), that component cannot be the focused component, right?

Anyway, I will leave it to you to resolve this bug.

[prissi]

I was aware of the unfocus problem, at least partly. My feeling is, that textinp and numberinp need to take care of the string/value in question and compare it to a copy. and only call the lsiteners, if both differ. However, in case of the savegame, calling with empty names could be just ignored ...

Actually, as far as now, there were less problems than I feared given my little free time to work and debug this stuff.

[knightly]

I was aware of the unfocus problem, at least partly. My feeling is, that textinp and numberinp need to take care of the string/value in question and compare it to a copy. and only call the lsiteners, if both differ. However, in case of the savegame, calling with empty names could be just ignored ...

I see. So you will add some more code later. But how about fetching focused component before calling infowin_event()?

[prissi]

In the original code there was a comment, that the component may be deleted after infowin. Also the focus was fetched beforehand. Thus I left it like this.

[knightly]

In the original code there was a comment, that the component may be deleted after infowin. Also the focus was fetched beforehand. Thus I left it like this.

I know, but I can't follow the logic of that comment -- that's why I asked. It says infowin_event() can delete "the component" -- what component does it refer to? Can you elaborate further on this?

[Dwachs]

This

Code Select Expand

komp->infowin_event(&ev2);

can delete the component komp.

This lead to a crash with the savegame window, therefore I removed the delete's there, and made the to-be-deleted components only invisible.

I do not know, which windows / elements are deleted in an infowin_event call followed by some action_triggered calls. If this can happen, then this code is highly unsafe.

Code Select Expand


         // Hajo: infowin_event() can delete the component
         // -> thus we need to ask first
         gui_komponente_t *focus = komp->get_focus();

         swallowed = komp->infowin_event(&ev2);

         // set focus for komponente, if komponente allows focus
         if(  focus  &&  IS_LEFTRELEASE(ev)  &&  komp->getroffen(ev->cx, ev->cy)  ) {
            /* the focus swallow all following events;
             * due to the activation action
             */
            new_focus = focus;
         }


-- if komp is deleted, then there is a big chance that focus is invalid (especially if get_focus returned simply 'this')
-- if komp is deleted, the call to komp->getroffen is invalid

I would rather propose to remove any code inside infowin_event's that deletes any element.

[prissi]

Well, in priciple this should not happen. No problem, if it is not something that can catch a focus. However, now buttons can catch focus too. On the other hand deletion should occus after event handling. I will lokk at the code a little more.

[Dwachs]

I just grepped for "delete" in gui/*.cc. Most of this calls are inside destructors (and can be removed since the components will be deleted by the destructor of gui_container too).

These calls may lead to deletes inside event processing:

Code Select Expand


-- in fabrik_info_t::update_info()
-- in halt_detail_t::halt_detail_info(cbuffer_t & buf)
-- in settings_stats_t::free_all()
-- in gui_scrolled_list_t::clear_elements()
-- in gui_scrolled_list_t::zeichnen(koord pos)

All of these incidents look rather innocent to me. The method gui_scrolled_list_t::clear_elements() is called on several occasions, but it seems it is used only for initialization.

[knightly]

@Dwachs

Thanks for your explanation.

Indeed, deleting the component referred to by komp in infowin_event() is very dangerous, which is something that I haven't thought of happening. And the focus would probably be invalid too. That's why I asked, as the order of the code looks suspicious.

After fixing the remove-component problems in infowin_event(), we should change the code to fetch focus after calling infowin_event(); otherwise, frames with inner pane(s) will not have the focused component immediately updated, like in the problem described above.

[prissi]

Thank you very much for digging into this.

[knightly]

@Prissi

There is a problem with r3470 : upon opening the load/save frame, when I press the TAB key, the textfield loses focus, receives a WIN_UNTOP event and tries to call its listeners, resulting in an attempt to load a game with empty name and the error message "Cannot load saved game!".

I understand that such unfocus=>action behaviour is desirable for situations like stop names, which should be immediately updated upon unfocus. However, it is not appropriate for load/save dialogs where there are more than one way of user input (i.e. user can input the file name in the textfield, or click on the button associated with a particular file).

I have fixed this problem by adding a flag to indicate whether unfocus will trigger calling of listeners in text input.

[prissi]

Since this is a very special situation, I would have rather changed savegame_frame.cc to not handle TAB and ESC ... because now I cannot load a game with the return key ...

EDIT: Moreover, since if I do not want callbacks, I should not add a callback function, i.e. this functionality exists already.

[knightly]

After your fix, hitting TAB now brings up the Keyboard Help dialog, which is not what users expect.

Besides, if you click on a file button, then drag and release outside that button, the dialog closes and the "Cannot load save game!" message is shown. In the past, such error message did not show up and the load/save dialog did not close. This is caused by the unfocus=>action behaviour of the text input. You may add code to guard against null strings, but still, the same problem happens if someone keys in some arbitrary text and afterwards tries the click-drag-release action on file buttons as mentioned above.

No matter how "special" you think such a case is, we really need a text input that does NOT trigger action upon unfocus. If such text input does not respond to the ENTER key, you can add some code in its infowin_event() to explicitly handle the ENTER key event, provided that the event is not handled indirectly on unfocus.



As a side note, it is rather strange that, upon opening a dialog, hitting TAB only brings up the Keyboard Help window, instead of focusing on the first focusable component. Also, keep hitting the TAB key will not bring you back to the first focusable component, but will eventually call up the Keyboard Help window. And, may I ask, will TAB traverse components of the inner containers?

[prissi]

TAB should transverse inner containers, since the inner container should swallow it.

The other issue is, that container returned NULL even though they could handle focus. The focus check needs to take care of potential focus too ... this would me on the other hand that always the first element (could be also the first textinp) recieves automatically a focus. Not sure, if this would not even the desired. (The dialogue could override it anyway manually.) Needs to make more consequent use of get_allow_focus().