search

riperiperi / FreeSO

Re-implementation of The Sims Online.
http://freeso.org
Mozilla Public License 2.0
817 stars 95 forks source link

Fixing Accidental Sale from Price Dialog #137

Closed thertzelle closed 5 years ago

thertzelle commented 5 years ago

This change prevents a player from accidentally deleting an item when setting the SalePrice via the delete key.

Most people are familiar with using a combination of delete and backspace in-order to edit text in a textfield, this will prevent the accident that everyone has done at least once, of pressing delete and selling a very expensive item.

Additionally one very small UI/UX enhancement was made, in that when you select "Set for Sale", and the dialog is presented, your cursor is automatically focused in and ready to adjust the price.

Implementation: I actually really thought about the best way to take care of this situation. My initial implementation used on focus events to track the dialog and the focus of the ForSalePrice UITextEdit. But in the end I wanted to make sure that the PriceDialog was getting de-allocated correctly, and remove focus. The submitted code seemed like the best of all worlds for memory management and user experience.

Tested in various states: Placing Multiple Items, Setting Multiple Items for Sale, Deleting after an item is set for sale or canceled.

geekgirl101 commented 5 years ago

<3

riperiperi commented 5 years ago

LGTM, but I'd like you to try something else before confirming.

Right now, it looks like pressing delete on something else (like the chat, bulletin board, direct message) will cause the same unintended effect. When the game is "active", InputManager's current focus is null. This is set whenever the player clicks onto the game. I noticed you made the price text autofocus - perhaps checking if the current focus is a textbox (or really anything) is enough to get it working in more cases?

A small problem I could see occurring is that the text box might remain focused after it is deleted (popup closed), but I could fix that with a sanity check in InputManager that unsets focus when it's on a removed element.

thertzelle commented 5 years ago

I will double check; Initially I coded all of this behavior into the change, and setting it to NULL seemed to make all the other changes pointless. So I'll do some very good testing tonight to ensure the expected behavior. Thanks!

thertzelle commented 5 years ago

SellBack() does get called still if your holding the item and in any other chat. However InputManagers GetFocus() function seems (and seemed) unpredictable. In that even if the parent dialog that is holding is deallocated...I'll have to give this a bit more thought, even if I lose focus on the price dialog, every other dialog is going to have this problem. 😢

I'll continue to look at better solutions tonight!

geekgirl101 commented 5 years ago

Uh oh, I really did give you quite a bit of a chore there didn't I? :(

thertzelle commented 5 years ago

I have updated the changes removing most of my code changes in favor of using exclusively. if (state.InputManager.GetFocus() == null)...To help facilitate the clearing of focus and a better UX, an additional change was added to "automatically" place the item back on the ground, rather than keeping it in the users hand. This removes the focus, but additional allows the user to price items faster.

riperiperi commented 5 years ago

LGTM, this is a lot cleaner. I'll try make the InputManager auto-detect when a focused element has been deleted to clear focus when that happens.