New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Added basic hotkey support and scroll speed and zoom speed options in Option menu. #1486

Closed
wants to merge 26 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@leiget
Copy link
Contributor

leiget commented Oct 17, 2018

I plan on expanding the hotkeys to include all in-game panels and being able to change them in the options menu.

leiget added some commits Oct 14, 2018

Added scroll speed, shift scroll speed, and zoom speed options to the…
… option menu. Added ability to change in-game hotkeys; no in-game window to change them, yet. Made diagonal scrolling the same speed as horizontal or diagonal scrolling speed (it's been normalized).
@TheCycoONE
Copy link
Member

TheCycoONE left a comment

It is probably also worth dropping key_mapping.txt and related code. It's purpose is essentially the same as what you are doing, and it is deeply flawed.

See #1438

Show resolved Hide resolved CorsixTH/Lua/app.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/app.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/config_finder.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/config_finder.lua
Show resolved Hide resolved CorsixTH/Lua/config_finder.lua
Show resolved Hide resolved CorsixTH/Lua/dialogs/resizables/options.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/game_ui.lua Outdated
Show resolved Hide resolved CorsixTH/CorsixTH.lua Outdated

@TheCycoONE TheCycoONE changed the title Added basic hotkey support and scroll speed and zoom speed options in Option menu. [RFC] Added basic hotkey support and scroll speed and zoom speed options in Option menu. Oct 18, 2018

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 18, 2018

Thank you @leiget

As mentioned above, some changes are requested - mostly related to formatting, a few more significant. I like what you are doing here and think it will improve the game. I hope you continue with this.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 18, 2018

Okay, thanks. I'll try to add code consistent with the current formatting.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented on CorsixTH/Lua/dialogs/bottom_panel.lua in db9ad36 Oct 28, 2018

Do not change the old afterLoad items for this. You will need to add a new condition to the end and bump App SAVEGAME_VERSION

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 28, 2018

Yeah, the afterLoad stuff is wrong. Basically in all cases now you want to remove all keymaps and set them all on load as well as initialization.

For each UI object with keymaps make a function to set keys and call it both at the end of afterLoad and in the constructor. Clear any mappings.

Show resolved Hide resolved CorsixTH/Lua/game_ui.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/ui.lua Outdated
Show resolved Hide resolved CorsixTH/Lua/utility.lua Outdated
@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 29, 2018

Okay, I'm still figuring this stuff out. What you are saying is that you want me to:

  • Change all the "addKeyHandler" keys back to how they were before.
  • Have them modified according to the hotkey.txt file when loading a save game and when initializing the app.
  • Have them modified by making a function to do so within every object with "addKeyHandler" is created.

Is that right?

@mugmuggy

This comment has been minimized.

Copy link
Contributor

mugmuggy commented Oct 29, 2018

Change back all the addKeyHandlers in afterLoad functions

Create a new 'handler' for afterLoad functions to remove all the old key bindings, then also register your new addKeyHandlers. This will be guarded by the oldversion < newversion check and and you change the SAVEGAME_VERSION see my previous PR #1314 just this isn't unregistering the old keyHandlers This covers your last 2 points. Old saves remove and register the new hotkeys binding, and new games, ignore the afterload change.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 29, 2018

Close @mugmuggy, but unfortunately in this case it can't be guarded because the keys could change. Just run the handler every time in afterLoad.

Ideally we wouldn't save them at all, but that can be future work.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 29, 2018

Okay, I get it.

So I'll need to bump SAVEGAME_VERSION to, say, 131, and then in an afterLoad class function I'll do basically something like:

if old < 131 then
addKeyHandler(hotkeys["ingame_ingame_panel_bankManager"])

Although I think that examples wrong?

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 29, 2018

Skip if old in this case, and also remove the old binding.

local function setKeyHandlers(ui)
  ui:addKeyHandler(hotkeys[...], callback)
  ...
end

function UIPlaceObjects:afterLoad(old, new)
   ...
   setKeyHandlers(self)
end

And in ui.lua

UI:afterLoad(old, new)
  self.key_handlers = {}
  UI:setupGlobalKeyHandlers()
  Window.afterLoad(self, old, new)
end

Since we are now eliminating all key handlers in all save games with the above, also remove any old afterLoad conditions that manipulate them across the code base.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 29, 2018

I'll do that. I have to go to work now 😢 but I'll get to work on it tomorrow. Thanks for the help.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 29, 2018

If you like later tonight I can write a patch file

@mugmuggy

This comment has been minimized.

Copy link
Contributor

mugmuggy commented Oct 29, 2018

Thanks @TheCycoONE I was coming back to fix up some of my comments. Question though as the key configs will be centralised to the file is it worthwhile passing the table still to add/removeKeyHandler rather than just the 'key' for the app.hotkeys lookup.

self:addKeyHandler(self.app.hotkeys["global_fullscreen_toggle_alt"], self, self.toggleFullscreen)

to

self:addKeyHandler("global_fullscreen_toggle_alt", self, self.toggleFullscreen)

and have the keyHandler routines deal with the lookup?

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 30, 2018

Commit that patches your afterLoad handling; doesn't address @mugmuggy's comment about just using names and doing the mapping in ui.lua (which I think is a good idea)

TheCycoONE@c78e59c

Let me know if you want me to apply it directly to your branch. (I only touch other people's pull requests with permission)

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 30, 2018

Yep, you can. I'm new to git: is it called a diff patch?

Merge pull request #1 from TheCycoONE/hotkey_load_fix
Fix hotkeys when loading save games
@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 30, 2018

I merged it already.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 30, 2018

I noticed that you got rid of some of the "if old < " statements that had addKeyHandler() functions in them. Do you want me to put those back in or ignore it?

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 31, 2018

They were removed intentionally. How afterLoad works is that when you load a save game, it calls the afterLoad functions (you can follow the path from app, up through the various objects) to convert the save to the current format. Since the current format doesn't use any of the old keypress handlers, we do not want to add them to save games, no matter how old. I explicitly drop them all on all save games in ui.lua.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 31, 2018

My current issue is that I'm trying to add a setKeyHandlers() function to be used at the end of World:World(), but the ui hasn't been setup yet, so that means I can't use addKeyHandler() or removeKeyHandler().

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Oct 31, 2018

Why can't it go in setUI where I put all the others?

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Oct 31, 2018

I'm confused. Are hotkeys saved when a level is saved, and then they are loaded when a save game is loaded?

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 3, 2019

I don't know much about git. I've looked up rebasing and merging and I can't make heads or tails of it. Just do what you think is best.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 3, 2019

Ok. This is not the PR I would suggest cutting your teeth on so I'll handle it now. Thank you for your persistence!

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 9, 2019

Hey. Just wondering what the status was on this PR. Anything you need me to do?

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 10, 2019

Nothing for you at the moment, unless you could rebase on the latest master to see if it fixes appveyor.

I need to test on Windows, then merge.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 12, 2019

I might as well try to rebase it.

Would I need to do this:

git rebase master

or

git rebase origin/master
@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 12, 2019

Neither probably. If you have a remote called upstream for CorsixTH/CorsixTH then git fetch upstream; git rebase upstream/master

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 13, 2019

I'm trying to rebase, but I can't seem to get it. I tried branching and other things, but the rebase doesn't seem to be doing anything.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 13, 2019

That last commit very much broke things... ok, just leave it to me.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 13, 2019

Lol, sorry. I should of just left i t alone. Do you need the original code? I mean my code.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 13, 2019

No, I can recover it.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 13, 2019

Okay, I won't mess with it anymore.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 15, 2019

Probably not what you want to hear, but I found a bug:

  1. Change some hotkeys
  2. Click accept
  3. Go back to hotkeys, change some more
  4. Click cancel

Instead of going back to the previously configured hotkeys, it goes back to the original set from when the game was loaded.

Want to fix this after the merge?

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 15, 2019

I go to bed in 3 hours, so I'll try to fix it tomorrow. If I'm not able to finish it by 1 pm EST, then I'll fix it after the merge.

Reverted back to working commit.
Fixed bug where if you were to cancel the hotkey assignment window it incorrectly reverts back to the hotkeys used when the program starts.
@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 15, 2019

Fixed it. And I wanted my fork to be back to working order so I pushed it again. Sorry if that makes things messier.

The changes I made to fix the bug you mentioned are in "/CorsixTH/Lua/dialogs/resizables/hotkey_assign.lua" It's just a hotkeys_backedUp = false in the accept buttons function, and I added a shallow_clone in the cancel buttons function so it would copy the backup table instead of just pointing to it, just in case not copying it causes problems.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 15, 2019

Ugh, and when I copied everything I forgot luacheck. Sorry again.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 16, 2019

Ok, I had started the rebase/merge. I'll pull your changes into it.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 17, 2019

Verified your fix

TheCycoONE added a commit that referenced this pull request Jan 18, 2019

Merge pull request #1486 from leiget
Added basic hotkey support and scroll speed and zoom speed options in Option menu.
@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 18, 2019

Merged

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Jan 18, 2019

Leiget, now that this is merged, please consider updating the wiki, specifically: https://github.com/CorsixTH/CorsixTH/wiki/Hotkeys and the relevant programming ideas section.

Also due to the way you used git, your private repo / master branch is going to be a mess. If you have upstream as a remote:

git fetch upstream
git reset --hard upstream/master
@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 18, 2019

Okay, I'll have to get on that tomorrow. As far as the git upstream thing you mention, I'm not sure I have upstream as remote.

@TheCycoONE

This comment has been minimized.

@leiget

This comment has been minimized.

Copy link
Contributor Author

leiget commented Jan 20, 2019

I edited the "Hotkeys" and "Programming Ideas" wiki pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment