Page MenuHomeWildfire Games

Refactor input.js
Needs ReviewPublic

Authored by nani on Mar 16 2019, 7:29 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Refactor from a switch to an unordered map a.k.a javascript object.
Benefits:

  • Cleaner code
  • No break; necessary
  • Each event type is his own function call
  • Same performance
  • Possible to change function event at runtime
Test Plan

Everything should behave as before.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7106
Build 11600: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Mar 25 2019, 10:11 PM
binaries/data/mods/public/gui/session/FSM.js
236

(would have said var -> let, but the file is a copy I guess and out of scope thus)

nani updated this revision to Diff 7622.Mar 26 2019, 5:27 PM

Cleanup

  • Added minor helper functions
  • Removed duplicated code
  • Made easier to add new input events
  • Reorganized input states to a more logical naming/structure

Fix

  • Box select (previously BANDBOXING) now works when camera moves
nani marked 8 inline comments as done and 3 inline comments as done.Mar 27 2019, 2:10 AM
nani added inline comments.
binaries/data/mods/public/gui/session/input_events.js
24

startsWitch is the intended behaviour.
Ex:
"A.B.C".startsWith("A.B") -> state inside of the active
"A.B.C".startsWith("A.B.C") -> state inside of the active
"A.B.C".startsWith("A.B.C.D") -> state not inside of the active
"A.B.C".startsWith("B.C") ->state not inside of the active

nani updated this revision to Diff 7623.Mar 27 2019, 2:22 AM
nani marked an inline comment as done.

Cleanup

  • Integrated all the ACTION_FOO into the FSM
  • Removed duplicated code
  • Moved most implementations specific methods to their corresponding files
  • Created corresponding functions calls to place/preview buildings/walls
  • Refactored double/triple click units selection

Stage

  • More o less now the FSM on itself seems done.

Now would be a matter of improving all the helper functions that the FSM calls.

nani updated this revision to Diff 7624.Mar 27 2019, 3:13 AM

Reupload (the formatter modified some unrelated lines)

nani updated this revision to Diff 7626.EditedMar 27 2019, 11:55 PM

Further cleanup, streamlined fsm states logic more.

nani updated this revision to Diff 7627.Mar 28 2019, 4:02 PM

Added a warning check

Harbormaster completed remote builds in B7057: Diff 7627.
nani added a comment.EditedMar 28 2019, 8:47 PM

For now this patch moves:

  • INPUT_FOO
  • ACTION_FOO
  • the two big switch that evaluate the states (old fsm)

Transforms it to:

  • A FSM that combines the INPUT_FOO and ACTION_FOO + the menu action
  • Decouple fsm logic from state functions (move them to their respective files if possible)
  • Reword state "NORMAL" "onmousedown" function to a simpler one (the 1/2/3 click part)

Tries to follow #5387 and #2826
Fixes #4848
Fixes selection box not working if camera moves

The fsm.js has been modified to be able to have a default function and "_" in state name

The fsm.js has been modified to be able to have a default function and "_" in state name

What uses the underscore?
Defaults are an anti-pattern, because
(1) it means the caller omits mentioning the value, so when someone searches for the value, he won't find all matches
(2) the behavior of the default value might go unnoticed by the caller (assuming that default always fits because that's why it's the default) but mentioning the value makes the reader more critical as to whether the value is correct, and informs the reader directly which value is used.
(3) is all calls explicitly state the value, then it allows comparison between the calls, and in some cases one might find a better value than the default
That argument is mostly for function argument defaults function x(a, b = 123), I didn't investigate what you did with this default here, but I expect it will match the same pattern.

19:31 < nani> FeXoR: how well versed are you in input.js file from session ?

Was it a lucky try? Because FeXoR is primarily a mapauthor, but I think he worked on the wallplacement code in this file, so perhaps you found him using the revision history. Might suceed with a ping @, but we have people who are more dedicated to the JS GUI, Imarok and bb would be a good bet, but one of them has broken bones and the other one is studying. Vladislav is another option, but he is also busy with various stuff. I guess I know another option to have this patch committed that involves me not committing it, but that would be new.

binaries/data/mods/public/gui/session/input.js
76

Notice that this might mean that the array is created everytime the function is called (unless the JIT compiler is smart).
The code inside that function to distinguish arrays from strings is also adding lots of complexity when the caller can just be has(x) || has (y).

In fact if there is a winning argument for having the array ["x", "y"], one could make it ["x", "y"].some(g_InputEvents.hasBaseState) (or .every), without the checks inside that function. Still think a simple || is the path through least complexity

219

let target;

231

the two lines can be merged

242

Avoid one-letter variables, because it makes the reader question what the content contains, what the reason was to for the author to chose this one particular letter (was it r for result?) instead of informing the reader what it means; and when the reader reads the later references to that one letter, is required to remember the definition instead of being able to evaluate the line by itself based on the variable name. (Yes those are only 2 lines, but it's an antipattern). Same for the (v, i) below. Either don't change the line, or remove all it's defects to minimize the total number of patches to the same line and maximize the quality per patch.

Also you can unify the two if-statements using result = x.y && x.y(z).

This function looks like it could be refactored, so that modders can insert new blocks in between the function without replacing the function, i.e. by moving every block to a separate object property and then handling that object here. Probably we don't want to do that now due to the complexity, so all of the syntax candy could be removed from this patch, making this patch shorter and then added to the refactoring one afterwards. Could.

314
  1. Oh noooo, I remember this function contains an anti-treasure
  2. Comments above code, so that every line is either a line of code or a line of comment (autism)
315

where does that space come from? The linter? The parentheses are unneeded as far as I know. Also we use === only when we need to distinguish == from === (leaving us with a codebase that has`== in almost any case and only === for few cases where falsy values need to be differentiated, instead of people chosing === and ==` depending on the angle of the sun)

347

I would have said remove the {}, but since you actually do change logic in this patch, I would rather advocate to split the parts that don't change anything, so that the reviewers of the proposal and the auditors of the commit are confronted with as little code as possible (trying to find the needle (bug) in the haystack is easier the less hay there is)

368

Define bandboxObject just above the first use.
I suspect the value won't differ from bandbox anytime soon, and if it would, we would need code to update all the bandbox objects, not only one of them. So it looks like fake freedom that results in unneeded indirection, but I might be wrong.

458–460
515–516

missing quotes, double negation sounds unneeded here (the trick is usually used to avoid warnings if a property is absent and return false)

646

I'm not sure if it's a good idea to ever put something other than numbers into a Vector2D, since the entire prototype consists of functions that expect numbers. Better pass undefined if you want to express the absence of a value.

716

If the states are always used as complete strings (x ? "PLACEMENT.Y" : "PLACEMENT.X") then one can do a stringsearch for "PLACEMENT.X" and find all matches, making it a bit easier to trace it down. (Same goes for filenames. People often used "foo" + bar + ".png", so when someone searches for that image file, it's impossible to tell which places can use it, except the code is foo ? "foobar.png" : "foobaz.png")

943

Didn't that object have the pattern { "ent": ent }? Then it would be a simple ents.map(ent => +ent) without Object.keys(). But again, better remove the syntax changes out and only do the logic changes here. If you want to please the linter with all the var/let and whitespace changes, make a patch that only contains these, so as to make it a very simple task to read.

989

Why is this moved to a separate function far away from the caller when all the other code remains inlined?

binaries/data/mods/public/gui/session/input_events.js
17

I wonder whether we can find a more expressive name than input events.
Input events are keypresses and mousemoves, but the logic in this file relates more to the logic of unit actions, building placement, etc...
The name should imply a clear definition to inform the reader what code can be expected to be found in this file, and prevents authors from adding unrelated stuff (clearly not everything that relates to input events is in this file, prevent spaghetti)

110

In what case is one of the two conditions true and the other false?

116

.some if indexOf doesn't cut it

120

{}

200

The other places use a switch instead of if for this case, so could make it more consistent

242

A function should either always or never return a value, i.e. return undefined or false here. (Yes return; is the same as return undefined; but the reader shall encounter return; for methods and return x; for functions where the return value is read from). We have a linter rule for that too

248

(!length)

297

variable can be inlined

313

the two variables can be inlined, which not only removes some characters, but also changes the structure of the function to be a sentence of the form subject predicate object, instead of object subject predicate.

480

Globals should be defined at the top of the file, so that it's the first thing readers read, and then read the functions that operate on it.
Code should not be run when the file is loaded, but upon an event (such as init function or a function called by that), so that it is more logically defined than dependent on the filename order where possible.

binaries/data/mods/public/gui/session/placement.js
32

()\n{
let
What is the best location in this file for this function?

42

position = snapData (.clone() unneeded since we receive a copy and don't read later from the value)

nani marked 18 inline comments as done.EditedMar 29 2019, 5:56 PM

What uses the underscore?

None but given that the state can only be written with uppercase it might serve in the future as a word separator given that it doesn't conflict with the naming of the methods (but anyway I can just revert this change for now)

Defaults are an anti-pattern, because
(1) it means the caller omits mentioning the value, so when someone searches for the value, he won't find all matches
(2) the behavior of the default value might go unnoticed by the caller (assuming that default always fits because that's why it's the default) but mentioning the value makes the reader more critical as to whether the value is correct, and informs the reader directly which value is used.
(3) is all calls explicitly state the value, then it allows comparison between the calls, and in some cases one might find a better value than the default
That argument is mostly for function argument defaults function x(a, b = 123), I didn't investigate what you did with this default here, but I expect it will match the same pattern.

The default allows to ignore all the events that one might not want to process but still happen (e.g. if in a state "PLACING" you only want to process "onmousemotion" event and ignore all the other types of events) otherwise the FSM will warn you every time you try to process an event in that state that is not defined. Mostly the default method should be an empty function in this FSM case (unless we are talking about those two "default" that I had to define at "PRESELECTED" and "SELECTING.BOX" states given that I could not find a way to refactor them to non-defaults methods.
All in all, these possible unknown "default" calls that might happen can be easily seen uncommenting a warning in the FSM.js file to see what is being called at each processed event.

19:31 < nani> FeXoR: how well versed are you in input.js file from session ?

Was it a lucky try? Because FeXoR is primarily a mapauthor, but I think he worked on the wallplacement code in this file, so perhaps you found him using the revision history. Might suceed with a ping @, but we have people who are more dedicated to the JS GUI, Imarok and bb would be a good bet, but one of them has broken bones and the other one is studying. Vladislav is another option, but he is also busy with various stuff. I guess I know another option to have this patch committed that involves me not committing it, but that would be new.

No luck

binaries/data/mods/public/gui/session/input.js
76

I suppose you are right. But the benefit of this built in method is that it checks if those states really exist (with warnIfStateNotDefined) so in the case someone wrote "PLAICNG" instead of the correct "PLACING" it will give a warning and a stack trace for better debugging.

242

I leave this for another patch.
Maybe all the var -> let change should too be another patch.

314

anti-treasure ?

315

Idk from where that space came from.

458–460

Anything is possible but why?

515–516

These two functions beforeGUI/afterGUI always expect a return true/false (because of c++ I think)

646

The original were undefined too so I must follow as I only change them to a vector2d format

716

Indeed

989
  • Because it doesn't affect the flow of the FSM (change of state )
  • What other code ?
binaries/data/mods/public/gui/session/input_events.js
17

I agree, any suggestions? My naming skills is my weak point :)

110
120

Ok, but come on, does it really matter ?

313

Shorter lines are better no? Having a s/p/o order doesn't matter is is more difficult to read.

480

Ok. Will be sent to input.js

binaries/data/mods/public/gui/session/placement.js
32

Right here looks fine.

nani marked 8 inline comments as done.Mar 29 2019, 6:55 PM

Btw, thanks for reviewing.

The default allows to ignore all the events that one might not want to process but still happen (e.g. if in a state "PLACING" you only want to process "onmousemotion" event and ignore all the other types of events) otherwise the FSM will warn you every time you try to process an event in that state that is not defined. Mostly the default method should be an empty function in this FSM case
All in all, these possible unknown "default" calls that might happen can be easily seen uncommenting a warning in the FSM.js file to see what is being called at each processed event.

So one of the problems is that all input events such as keydown, mousedown can be triggered, the FSM model expects all these to be defined, but we don't want to specify tons of empty functions? Would it actually be that many empty functions? Some empty lines here and there wouldn't hurt so much and it could possibly yield a more complete picture. Empty call handlers could also be considered as an incentive to think about filling them.
One can consider switch to not complain about missing functions.
One has to see which of the two solutions is better for this prototype, can't generalize.

(unless we are talking about those two "default" that I had to define at "PRESELECTED" and "SELECTING.BOX" states given that I could not find a way to refactor them to non-defaults methods.

The events have entirely different arguments (keydown the button, mousemove the coordinates), so the arguments of the default function is unable to consume the provided argument.
If one reads "keydown", "mousemove", it's immediately clear to the reader what case this function handles. But it's not clear which cases "default" handles. Seems that was the default from the switch statements, so that must be every event other the specified ones then.
Regardless whether its a default in the switch or a default function, the reader would have to know all possible events to be able to tell which cases the default case handles.
If every case was handled, it would be clear. But that might lead to duplication in some cases. So it depends on how much duplication that would be (calls to a helper function are still few duplication compared to the default approach).
If we know how much duplication it would be, and if it would be significant enough, we can keep the default for lack of a better idea.

Isn't InputEvents.prototype.events.default unneeded either way?

Why are unit actions (patrol, gather, ...) mixed with input events (keydown, mousemove, ...) for some objects?

19:31 < nani> FeXoR: how well versed are you in input.js file from session ?

Was it a lucky try?

No luck

(I meant whether you had found him by looking through the authors of this file)

In D1786#73631, @nani wrote:

Btw, thanks for reviewing.

You're welcome. As mentioned in a previous post, I post my comments to help educate about good practices, but I'm currently not working on code until I fixed some problems with working on code that I had over the years.

binaries/data/mods/public/gui/session/input.js
76

Whether or not it warns is independent of whether the function supports arrays

192

(x,y) don't exist anymore, also it's not clear whether those are screen coordinates or a map position. Should state that the position is the location that the user has clicked on the minimap.

pos -> position (U dont rite like dis fo a reasn)

Perhaps
pos -> minimapPosition

314

The price of discovering it is reduced lifetime

315

Sounds like the editor

457–458

null is useful to distinguish undefined in a condition.
Why use the value range (object, undefined, null) when (object, undefined) is sufficient. We don't have true, false, totally false either unless there is a really good reason to.

Regardless, !hoveredObject is even shorter.

I thought it was an unused variable, but it's a global. I didn't check the number of references to mouseIsOverObject, but it should be correted if it's not many.

Even better than making it a global would be to make it a property of the prototype instance.

One may consider storing hoveredObject right away. At least it sounds slightly easier to keep in mind than mouseIsOverObject. hoveredObject refers to one thing, mouseIsOverObject refers to one thing and a property of that thing.
Storing the hoveredObject might also open up new use cases later. Hypothetical though.

458–460

Shorter formulas can be easier to understand, here it's just equal.
It's easy to dismiss minimization for one or two statements, but excess code does accumulate, often one could remove more than 1/3 to 2/3 of the code. That are several thousand lines code.

if (X)
	return false;
return Y;

<=>

return X ? false : Y;

<=>

return !X ? Y : false;

<=>

return !X && Y;

<=>

return !g_InputEvents.isStateAfterGUI() && g_InputEvents.ProcessMessage({ "type": ev.type, "ev": ev });

So handleInputBeforeGui is truthy if !isStateAfterGUI && ProcessMessage, exactly like the functionname suggests. The reader doesn't have to infer this anymore, but can read it directly.

Authors have the choice to leave code as concise, simple and clean as possible, and thus always try to resolve the equations, or they consciously chose to not even try but keep what works. That's the path to the dark side, spaghetti mess, and creating new works by copying and modifying previous works.

515–516

GUIManager.cpp

		if (top()->GetScriptInterface()->CallFunction(global, "handleInputAfterGui", handled, *ev))

That is defined in the lovely NativeWrapperDefns.h.

So bool handled is the return value.

The return value is captured as a JS::Value in jsRet and then converted to the specified type in FromJSVal.

FromJSVal<bool> is defined in ScriptConversions.cpp and it contains WARN_IF_NOT(v.isBoolean(), v);. I guess that's the reason why we need a bool?

(I don't recall the JS Interface being so picky)

645

unnecessary variable, inline

646

Doesn't change that Vector2D is designed for numbers.
Just pass undefined instead of Vector2D(undefined, undefined).
PickEntityAtPoint seems to be the only consumer, and that also expects numbers.
As established in the other inline comment, I expect that it throws a warning if it receives undefined instead of a number.

So determineAction now has the choice to assume or ensure that it will never PickEntityAtPoint with pos = undefined. If it would be called, then previously it would throw a warning, now it would throw an error, or a condition is added to account for pos = undefined.

So unless it becomes clear that determineAction can never be call PickEntityAtPoint with an pos = undefined, then we even discovered a defect in the previous code by making that a Vector2D.
.......

Looking at fromMinimap, it actually that does become clear.

If I'm not mistaken, we only need to pass a position variable if the user clicked on the minimap, or undefined if he didn't (women don't play).
So there's another advantage of the Vector2D, fromMinimap can be removed, only need to pass one argument instead of three,
and the condition for PickEntityAtPoint becomes rock solid, whereas before it seemed a bit fragile. (It wasn't clear that position is given if and only if fromMinimap).

972

(Hiding these globals here at the bottom is terrible, someone with commit access should really move them to the top)

989

The function is called from L215.
The function could be defined in L118 or L460, but it's at L944, why? Keeping related code grouped minimizes fragmentation.

Because it doesn't affect the flow of the FSM (change of state )

Usually one moves code to a new function to break a long function into smaller parts, so as to make them easier to understand separately.
These are 20 lines, so maybe considerable, but the function that calls it now only has one line, so nothing really gained?
The function is also called only from this one location (a hypothetical second caller can still call the function through foo.bar.baz.hotkey()).

If it's really only the absence of an FSM state that favors moving it, there are some other functions that also don't change the FSM state.
One can also split the other functions that do change the FSM state to helper functions that are called, changing the FSM state depending on the return value of the helper function.

binaries/data/mods/public/gui/session/input_events.js
17

The name should express the purpose of the prototype.

120

It's merely a consistency argument. If a reader reads 10x one variant, 5x the second variant, and 15x the other variant, he will notice the difference, wonder why it's the case. Only 5 seconds passed, but he will have the same thought everytime he goes through these. It takes up brain resources, instead the reader should direct all attention towards the original reason he had opened the file. (In general, if one keeps getting the same thought unrelated from the problem that one intended to work on, again and again when reading a file, there is probably something to that thought, fixing it removes the thought, sets one free. I need to test myself for autism some day.)

220

; same for the other lines of that content

313

Why would it be more difficult to read? I've heard that one often before, but it's a matter of habitualizing themselves to things without considering the alternative.

	let rect = updateBandbox("bandbox", msg.ev, false);
	let ents = Engine.PickPlayerEntitiesInRect(...rect, g_ViewedPlayer);
	g_Selection.setHighlightList(getPreferredEntities(ents));
	g_Selection.setHighlightList(getPreferredEntities(
		Engine.PickPlayerEntitiesInRect(...updateBandbox("bandbox", msg.ev, false),
		g_ViewedPlayer)));

I would claim the latter is easier to read,
and has the advantage that it the first thing that the reader reads is the purpose of the code setHighlightList,
rather than first challenging the reader to evaluate some helper variables before getting to know the cause for the helper variables to exist.

It reads:

Highlight the preferred entities in the bandbox of that player.

Instead of:

In the bandbox the entities of the player, highlight the preferred ones.

I've received a lot of hate for pointing this out, but it's the truth.
Code grows historically, we should not claim that to be the best choice merely because we came up with that variant first. Should always consider both alternatives, the pros and cons. It matters a lot if we chose one code style over the other in tens of thousands of lines. So we better pick the alternative that is best in the long term and leaves the code that is easier to read, even if some readers are habitualized to weird code.
I started coding in 2001 with QBASIC and that had no classes, no functions, only countless GOTOs (at least the code I was looking at). I got used to it, so I was unfamiliar with procedural and object oriented coding, so I could have claimed that was harder to read based on that. But if one wraps one head around it once, it's easier to have it cleaner, even if it takes some more steps to (re)write after obtaining the first working solution.

If one can order the statements logically and linearily (1 > 2 > 3 > 4), it's much more straight forward to read than 4, 2, 1, 3, even if the code result is the same. The more information we can transport to the reader, and the quicker the information is transported, the quicker the reader can decide on it, and the more qualified the decision will be, and he won't have to solve puzzles to start understanding and deciding.

binaries/data/mods/public/gui/session/placement.js
32

Wondering whether it's not better to keep it near the two callers to remove fragmentation

nani marked 15 inline comments as done and 9 inline comments as done.Apr 5 2019, 2:59 AM
nani added inline comments.
binaries/data/mods/public/gui/session/input.js
76

hasBaseState prototype provides with the desired solution (being able to compare against multiple states or only just one) and makes guarantees that the comparison is between states and not some random strings.

646

Nice

nani updated this revision to Diff 7672.Apr 5 2019, 3:05 AM
nani marked an inline comment as done.

Follow up

Harbormaster completed remote builds in B7105: Diff 7672.
nani updated this revision to Diff 7673.Apr 5 2019, 3:06 AM

Follow up

Preliminary note: If you can, move the non-refactoring changes outside of this diff. It's big enough. Git is convenient for this.

I am not sure this is the right way to go about refactoring input.js. The switch-as-FSM isn't the big issue here, the issue is that everything is a global and it's a huge mess.
I feel like it would be a better direction to try and move [actions] as a unit in their own separate file (for example tributing here remains mostly in menu.js with some bits in the FSM, which makes it very ugly). And then we'll actually have readable code, even if some of the scaffolding bits are a tad ugly, and then we can actually confidently refactor the scaffolding.

Since all actions appear revert back to "Normal" (i.e. none switch to another state / at most they switch to one of their own sub-state), I don't think we need an FSM.
I'm going to try and write a diff for that idea, but overall I'm not super confident this is the way to go.

Didn't look closely at it, but we definitely shouldn't copy the general fsm definition, but instead put it somewhere where UnitAI and input.js can use it.