Page MenuHomeWildfire Games

Session: Show more command buttons
Needs ReviewPublic

Authored by Imarok on Jan 28 2018, 10:52 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#4274
Summary

As decided in the staff meeting on 12. 02. 2017 (yes it took me "some" time...) we want to grant access to all command buttons and not only the first 6 by using little arrows.
This diff implements such thing. It's mainly up for discussion and agreement. Icon arrows are also missing.

Test Plan

Select a building and a unit together (or activate the back-to-work button of a unit) and test it.

Event Timeline

Imarok created this revision.Jan 28 2018, 10:52 PM
Vulcan added a subscriber: Vulcan.Jan 28 2018, 11:43 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 490| 490| 				continue;
| 491| 491| 
| 492| 492| 			if (state.pack.progress == 0)
| 493|    |-			{
|    | 493|+			
| 494| 494| 				if (state.pack.packed)
| 495| 495| 					checks.unpackButton = true;
| 496| 496| 				else
| 497| 497| 					checks.packButton = true;
| 498|    |-			}
|    | 498|+			
| 499| 499| 			else if (state.pack.packed)
| 500| 500| 				checks.unpackCancelButton = true;
| 501| 501| 			else

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 772| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
bb added a subscriber: bb.Jan 30 2018, 12:48 AM

Having multiple pages for it, seems the way to go on this

binaries/data/mods/public/gui/session/selection_panels.js
153

maybe make a this.itemPages for the 2

binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
14–18

instead of hardcoding two pages, perhaps put that in a repeat aswell, and then have every page with 6 items

Imarok updated the Trac tickets for this revision.Jan 30 2018, 4:52 PM
Imarok updated this revision to Diff 5592.Jan 30 2018, 5:04 PM

Let it be uncountable pages. (Also adapt XML and js, so that we can show all possible 13 buttons we currently have)

bb added inline comments.Jan 30 2018, 5:55 PM
binaries/data/mods/public/gui/session/selection_panels.js
104–105

maybe these should be moved to the top of the file (not sure for this file), also some jsdocs perhaps

165

the number of pages could fluctuate over time, and can be 3 to 2 so I guess you mean

if (numberOfPages < g_OpenCommandPage)
  g_OpenCommandPage = max(1, numberOfPages);

(not sure if that 1 is required)

168–169

if there are no pages, at least one of them will bug

I guess the max(1,foo) from above should be removed and check for <=1 here

binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
14

they end up as black squares now, some arrow would look better (I know there are space problems...)

15

We rely here on the onTick/onSimupdates for updating the buttons and those can take an arbitrary amount of time, instead of doing it directly by updateSelectionDetails

Imarok marked 4 inline comments as done.Jan 30 2018, 6:24 PM
Imarok added inline comments.
binaries/data/mods/public/gui/session/selection_panels.js
104–105

Thought I leave them her, because they are only used here...

165

true, I still had only 2 pages in mind ^^

168–169

if there are no pages, at least one of them will bug

no pages <=> no items => panel won't be shown

I guess the max(1,foo) from above should be removed and check for <=1 here

which max?

binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
14

they end up as black squares now, some arrow would look better (I know there are space problems...)

See the summary of this diff:

This diff implements such thing. It's mainly up for discussion and agreement. Icon arrows are also missing.

Imarok updated this revision to Diff 5593.Jan 30 2018, 6:24 PM
Imarok marked 4 inline comments as done.

See the comments.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 490| 490| 				continue;
| 491| 491| 
| 492| 492| 			if (state.pack.progress == 0)
| 493|    |-			{
|    | 493|+			
| 494| 494| 				if (state.pack.packed)
| 495| 495| 					checks.unpackButton = true;
| 496| 496| 				else
| 497| 497| 					checks.packButton = true;
| 498|    |-			}
|    | 498|+			
| 499| 499| 			else if (state.pack.packed)
| 500| 500| 				checks.unpackCancelButton = true;
| 501| 501| 			else

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 772| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 497| 497| 				continue;
| 498| 498| 
| 499| 499| 			if (state.pack.progress == 0)
| 500|    |-			{
|    | 500|+			
| 501| 501| 				if (state.pack.packed)
| 502| 502| 					checks.unpackButton = true;
| 503| 503| 				else
| 504| 504| 					checks.packButton = true;
| 505|    |-			}
|    | 505|+			
| 506| 506| 			else if (state.pack.packed)
| 507| 507| 				checks.unpackCancelButton = true;
| 508| 508| 			else

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 779| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

(ugly) arrows:


Needed images:

Owners added a subscriber: Restricted Owners Package.Feb 3 2018, 10:28 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 497| 497| 				continue;
| 498| 498| 
| 499| 499| 			if (state.pack.progress == 0)
| 500|    |-			{
|    | 500|+			
| 501| 501| 				if (state.pack.packed)
| 502| 502| 					checks.unpackButton = true;
| 503| 503| 				else
| 504| 504| 					checks.packButton = true;
| 505|    |-			}
|    | 505|+			
| 506| 506| 			else if (state.pack.packed)
| 507| 507| 				checks.unpackCancelButton = true;
| 508| 508| 			else

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 779| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Stan added a subscriber: Stan.Feb 3 2018, 11:36 PM

There is no arrow in the game at all ?

Imarok added a comment.Feb 4 2018, 6:53 PM
In D1269#52139, @Stan wrote:

There is no arrow in the game at all ?

True, there is https://github.com/0ad/0ad/blob/master/binaries/data/mods/public/art/textures/selection/arrow/128x128.png
I'll rotate that and try it out.

In D1269#52132, @Imarok wrote:

(ugly) arrows:



Needed images:

don't you think that buttons are too small to click. it could be the same size with the others and it could be a "forward" icon instead of narrowed play button. loves and kisses. xoxox

Imarok updated this revision to Diff 5821.EditedFeb 17 2018, 10:38 PM

Update images

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 497| 497| 				continue;
| 498| 498| 
| 499| 499| 			if (state.pack.progress == 0)
| 500|    |-			{
|    | 500|+			
| 501| 501| 				if (state.pack.packed)
| 502| 502| 					checks.unpackButton = true;
| 503| 503| 				else
| 504| 504| 					checks.packButton = true;
| 505|    |-			}
|    | 505|+			
| 506| 506| 			else if (state.pack.packed)
| 507| 507| 				checks.unpackCancelButton = true;
| 508| 508| 			else

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 779| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

Link to build: https://jenkins.wildfiregames.com/job/differential/37/display/redirect

Stan added a comment.Feb 18 2018, 1:26 PM

That doesn't look good :/

In D1269#53535, @Stan wrote:

That doesn't look good :/

Agree.
But I am no artist and this is the best I could reach for, so an artist (hint, hint ;P) should do a better icon ;)

In D1269#53945, @Imarok wrote:
In D1269#53535, @Stan wrote:

That doesn't look good :/

Agree.
But I am no artist and this is the best I could reach for, so an artist (hint, hint ;P) should do a better icon ;)

I can help to turn that ugly button into a better-looking button if you give me the source file.

In D1269#53945, @Imarok wrote:
In D1269#53535, @Stan wrote:

That doesn't look good :/

Agree.
But I am no artist and this is the best I could reach for, so an artist (hint, hint ;P) should do a better icon ;)

I can help to turn that ugly button into a better-looking button if you give me the source file.

Source file of what?

In D1269#53972, @Imarok wrote:
In D1269#53945, @Imarok wrote:
In D1269#53535, @Stan wrote:

That doesn't look good :/

Agree.
But I am no artist and this is the best I could reach for, so an artist (hint, hint ;P) should do a better icon ;)

I can help to turn that ugly button into a better-looking button if you give me the source file.

Source file of what?

I need the png file of one of those icons. I'm asking to understand the size of icons then I can come up with some ideas.

In D1269#53972, @Imarok wrote:
In D1269#53945, @Imarok wrote:
In D1269#53535, @Stan wrote:

That doesn't look good :/

Agree.
But I am no artist and this is the best I could reach for, so an artist (hint, hint ;P) should do a better icon ;)

I can help to turn that ugly button into a better-looking button if you give me the source file.

Source file of what?

I need the png file of one of those icons. I'm asking to understand the size of icons then I can come up with some ideas.

They are uploaded on this page. Just look a bit below this post...

I'm not sure if they look exactly like this in the menu because I added them on ps. Anyway, we can discuss their shapes and colors. I think it should be a "forward button" instead of a "play button". That's why the shape is like this and the color should be in an attention color like yellow, orange and red. It needs to say "there are more buttons" clearly. Well, I can work on it after we discuss it.

I'm not sure if they look exactly like this in the menu because I added them on ps. Anyway, we can discuss their shapes and colors. I think it should be a "forward button" instead of a "play button". That's why the shape is like this and the color should be in an attention color like yellow, orange and red. It needs to say "there are more buttons" clearly. Well, I can work on it after we discuss it.

In D1269#54037, @Imarok wrote:

I'm not sure if they look exactly like this in the menu because I added them on ps. Anyway, we can discuss their shapes and colors. I think it should be a "forward button" instead of a "play button". That's why the shape is like this and the color should be in an attention color like yellow, orange and red. It needs to say "there are more buttons" clearly. Well, I can work on it after we discuss it.

There is too much deformation here. I think the icon needs more space to make it wider if it's possible.

It might be nice to also be able to scroll to change the "page"?

Stan added a comment.Thu, Jun 25, 12:41 AM

I can offer this

I can also probably make highlighted versions if needed.

binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
22

Since the OOP rewrite, maybe this can be done nicely in the JS?

Imarok added inline comments.Thu, Jun 25, 12:54 AM
binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
22

Good question

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'state'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 317| 317| 		if (!g_AvailableFormations.has(unitEntStates[0].player))
| 318| 318| 			g_AvailableFormations.set(unitEntStates[0].player, Engine.GuiInterfaceCall("GetAvailableFormations", unitEntStates[0].player));
| 319| 319| 
| 320|    |-		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity &&  state.identity.formations.indexOf(formation) != -1));
|    | 320|+		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity && state.identity.formations.indexOf(formation) != -1));
| 321| 321| 	},
| 322| 322| 	"setupButton": function(data)
| 323| 323| 	{
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 780| 780| 				addResearchToQueue(data.item.researchFacilityId, t);
| 781| 781| 			})(tech);
| 782| 782| 
| 783|    |-			button.onPressRight = (t => function () {
|    | 783|+			button.onPressRight = (t => function() {
| 784| 784| 				showTemplateDetails(
| 785| 785| 					t,
| 786| 786| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

binaries/data/mods/public/gui/session/selection_panels.js
|  50| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  61| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 748| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2522/display/redirect

Imarok marked an inline comment as done.Thu, Jun 25, 1:20 AM
Imarok added inline comments.
binaries/data/mods/public/gui/session/selection_panels_middle/unit_commands.xml
22

Hmm I don't see how this could benefit from doing it in js only.

Imarok updated this revision to Diff 12454.Thu, Jun 25, 1:20 AM

Use icons from Stan. No extra icon definition. Remove change crept in from other diff.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'state'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 317| 317| 		if (!g_AvailableFormations.has(unitEntStates[0].player))
| 318| 318| 			g_AvailableFormations.set(unitEntStates[0].player, Engine.GuiInterfaceCall("GetAvailableFormations", unitEntStates[0].player));
| 319| 319| 
| 320|    |-		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity &&  state.identity.formations.indexOf(formation) != -1));
|    | 320|+		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity && state.identity.formations.indexOf(formation) != -1));
| 321| 321| 	},
| 322| 322| 	"setupButton": function(data)
| 323| 323| 	{
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 780| 780| 				addResearchToQueue(data.item.researchFacilityId, t);
| 781| 781| 			})(tech);
| 782| 782| 
| 783|    |-			button.onPressRight = (t => function () {
|    | 783|+			button.onPressRight = (t => function() {
| 784| 784| 				showTemplateDetails(
| 785| 785| 					t,
| 786| 786| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

binaries/data/mods/public/gui/session/selection_panels.js
|  50| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  61| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 748| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2523/display/redirect

It's a bit of a pain to quickly click on them now, perhaps show five at a time and increase the size of the arrows?

The benefit of the OOP is that e.g. the diff to scroll through the loading screen tips can partly use the same code. But @elexis would be way better at explaining that ^^

binaries/data/mods/public/gui/session/selection_panels.js
112

+\n.

Stan added a comment.Thu, Jun 25, 9:47 AM

Highlighted versions.

Tested and functional, I would agree with Freagarach that perhaps only showing 5 and having bigger arrows is easier to click though. Keep showing 6 in the regular case.