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

Add auto complete for "showScreen" command #2286

Merged
merged 4 commits into from Apr 13, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Apr 9, 2016

Contains

As title says, adds suggester for showScreen command. I couldn't resist to add this one. :)

How to test

Open console, write showScreen and hit TAB, it should be able show available assets for selection.

Demonstration

screen shot 2016-04-09 at 17 14 44

@GooeyHub GooeyHub added the in progress label Apr 9, 2016

@tdgunes tdgunes changed the title from Add auto complete for showScreen command to Add auto complete for "showScreen" command Apr 9, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 9, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/581/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 9, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/581/
Hooray Jenkins reported success with all tests good!

@rzats rzats added the UI label Apr 9, 2016

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Apr 9, 2016

Member

Nice! Code looks good to me. Haven't tested it (yet). Does this work with qualified names as well?

Member

msteiger commented Apr 9, 2016

Nice! Code looks good to me. Haven't tested it (yet). Does this work with qualified names as well?

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 9, 2016

Member

Thanks @msteiger, sorry but what do you actually mean by "qualified names"?

Member

tdgunes commented Apr 9, 2016

Thanks @msteiger, sorry but what do you actually mean by "qualified names"?

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Apr 9, 2016

Member

I meant to ask whether it (still) works, if you add the module name in front.
Example: engine:migTestScreen.

I just spotted that you're using UIWidget, which probably results in large amounts of false positives. Maybe you can narrow it down by using ScreenLayer or sth. similar?

Member

msteiger commented Apr 9, 2016

I meant to ask whether it (still) works, if you add the module name in front.
Example: engine:migTestScreen.

I just spotted that you're using UIWidget, which probably results in large amounts of false positives. Maybe you can narrow it down by using ScreenLayer or sth. similar?

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 9, 2016

Member

@msteiger it works, but auto completion only works with resource name.
For narrowing down the results to UIScreenLayer, I didn't notice that definitely find a fix for it.

@msteiger do you think auto completion should make suggestions with module name or without module name or both (with module name and resource name)?

The problem with both is that, there are lot's of duplicates:

screen shot 2016-04-10 at 00 45 19

Member

tdgunes commented Apr 9, 2016

@msteiger it works, but auto completion only works with resource name.
For narrowing down the results to UIScreenLayer, I didn't notice that definitely find a fix for it.

@msteiger do you think auto completion should make suggestions with module name or without module name or both (with module name and resource name)?

The problem with both is that, there are lot's of duplicates:

screen shot 2016-04-10 at 00 45 19

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 9, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/586/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 9, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/586/
Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 10, 2016

Member

Tested it out real quick and I can confirm that it works, although now only with fully qualified command names :-)

For me the most intuitive would be without module/engine name unless there's an ambiguous screen name. But is that where the dupes come in?

Member

Cervator commented Apr 10, 2016

Tested it out real quick and I can confirm that it works, although now only with fully qualified command names :-)

For me the most intuitive would be without module/engine name unless there's an ambiguous screen name. But is that where the dupes come in?

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 10, 2016

Member

For instance, if there is engine:migTestScreen and newModule:migTestScreen, both should be visible or else only migTestScreen, is that what you are trying say? In this way, there will be no duplicates, I guess.

Member

tdgunes commented Apr 10, 2016

For instance, if there is engine:migTestScreen and newModule:migTestScreen, both should be visible or else only migTestScreen, is that what you are trying say? In this way, there will be no duplicates, I guess.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 10, 2016

Member

@tdgunes exactly :-)

If you were to hit tab with showScreen mig present it would be nice if the logic was smart enough to pick either the engine or newModule option (starting at random or whatever) but leave both entries in the list so you can tab through them.

Member

Cervator commented Apr 10, 2016

@tdgunes exactly :-)

If you were to hit tab with showScreen mig present it would be nice if the logic was smart enough to pick either the engine or newModule option (starting at random or whatever) but leave both entries in the list so you can tab through them.

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 10, 2016

Member

@Cervator that would be great to have! :)

For testing, how can I create a newModule:migTestScreen?

Member

tdgunes commented Apr 10, 2016

@Cervator that would be great to have! :)

For testing, how can I create a newModule:migTestScreen?

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 10, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/588/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 10, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/588/
Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 10, 2016

Member

For testing, how can I create a newModule:migTestScreen?

Not really my expertise but I'm hoping you could just make a copy of engine/src/main/resources/assets/ui/migTestScreen.ui and drop it straight into a similar path of a test module then run the game and see if it shows up? The file has engine:migTestScreen in it so just change that part to someModule:migTestScreen

Maybe that's naive of me to suggest or perhaps the engine really is that easy to use, unsure which :D

Member

Cervator commented Apr 10, 2016

For testing, how can I create a newModule:migTestScreen?

Not really my expertise but I'm hoping you could just make a copy of engine/src/main/resources/assets/ui/migTestScreen.ui and drop it straight into a similar path of a test module then run the game and see if it shows up? The file has engine:migTestScreen in it so just change that part to someModule:migTestScreen

Maybe that's naive of me to suggest or perhaps the engine really is that easy to use, unsure which :D

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 10, 2016

Member

Oh ok, :). I didn't think it would be that easy. By the way, is there a mechanism to state that "Terasology, please load this module in start"?

In theory, my latest commit generates suggestions just like I mentioned above. But I wanted to be sure whether it works or not.

Member

tdgunes commented Apr 10, 2016

Oh ok, :). I didn't think it would be that easy. By the way, is there a mechanism to state that "Terasology, please load this module in start"?

In theory, my latest commit generates suggestions just like I mentioned above. But I wanted to be sure whether it works or not.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 11, 2016

Member

Oh ok, :). I didn't think it would be that easy. By the way, is there a mechanism to state that "Terasology, please load this module in start"?

As in, loading a module in the main menu so you can see things from the console there? I don't think so at present. We'll need that eventually for modules that reskin or otherwise change the menus.

Member

Cervator commented Apr 11, 2016

Oh ok, :). I didn't think it would be that easy. By the way, is there a mechanism to state that "Terasology, please load this module in start"?

As in, loading a module in the main menu so you can see things from the console there? I don't think so at present. We'll need that eventually for modules that reskin or otherwise change the menus.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 13, 2016

Member

I went ahead and tested this again just to be sure and all looks well :-)

Can confirm migTestScreen shows up unqualified if there is only one, and qualified if there are two (simply copied the .ui to Core) with the game running. Both work and open the UI.

With two options you can't auto-complete with mig<tab> since at that point it starts with either engine or Core - did you intend to find a way to make that work, @tdgunes? I'm happy merging this as-is since it is a nice addition and probably a rare use case to have identically named UIs in several places :-)

Member

Cervator commented Apr 13, 2016

I went ahead and tested this again just to be sure and all looks well :-)

Can confirm migTestScreen shows up unqualified if there is only one, and qualified if there are two (simply copied the .ui to Core) with the game running. Both work and open the UI.

With two options you can't auto-complete with mig<tab> since at that point it starts with either engine or Core - did you intend to find a way to make that work, @tdgunes? I'm happy merging this as-is since it is a nice addition and probably a rare use case to have identically named UIs in several places :-)

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 13, 2016

Member

Nice to hear that it works! :-)

I think making it work like that way, requires some changes in suggestion system. It seems that suggestion system only start auto completion from beginning. I will definitely look into that in future, when I have some free time.

This PR will be extremely helpful for me while testing #2288 (hate typing same thing over and over again :-) ), it would be great if you can merge it.

Member

tdgunes commented Apr 13, 2016

Nice to hear that it works! :-)

I think making it work like that way, requires some changes in suggestion system. It seems that suggestion system only start auto completion from beginning. I will definitely look into that in future, when I have some free time.

This PR will be extremely helpful for me while testing #2288 (hate typing same thing over and over again :-) ), it would be great if you can merge it.

@Cervator Cervator added this to the v1.0.1 milestone Apr 13, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 13, 2016

Member

Works for me! Thanks :-)

Member

Cervator commented Apr 13, 2016

Works for me! Thanks :-)

@Cervator Cervator merged commit 9bda456 into MovingBlocks:develop Apr 13, 2016

1 check passed

default Build finished. 473 tests run, 0 skipped, 0 failed.
Details

@Cervator Cervator removed the in progress label Apr 13, 2016

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