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 search to advanced settings #4806

Merged
merged 3 commits into from Jan 18, 2017

Conversation

Projects
None yet
@rubenwardy
Member

rubenwardy commented Nov 23, 2016

screenshot_2016-11-23_23-17-06

  • After search, the best result is selected
  • Clicking search or pressing enter in the text field causes the next result to be selected (like F3). This loops.
  • Keyword based, keywords can appear in any order or place and don't have to appear
@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Nov 23, 2016

Member

This is how the server tab search should look too :(

Member

sofar commented Nov 23, 2016

This is how the server tab search should look too :(

@kaeza

This comment has been minimized.

Show comment
Hide comment
@kaeza

kaeza Nov 24, 2016

Contributor

Tested. Works perfectly.

👍

Contributor

kaeza commented Nov 24, 2016

Tested. Works perfectly.

👍

@est31 est31 added this to the 0.4.15 milestone Nov 24, 2016

@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Nov 24, 2016

Contributor

I don't review Lua code very often so I won't. But for the concept, 👍 from me!

Contributor

Zeno- commented Nov 24, 2016

I don't review Lua code very often so I won't. But for the concept, 👍 from me!

@kaeza

This comment has been minimized.

Show comment
Hide comment
@kaeza

kaeza Nov 24, 2016

Contributor

In case before someone asks, and it is not clear enough, this supports localized strings, so non-english speakers may search settings in their native language.

Looking at the code, the only nits I have are the use of string.find(x, ...) instead of x:find(...), and probably how the elseif block at line 372 is split. Both of those are minor issues for me and it's fine either way.

Contributor

kaeza commented Nov 24, 2016

In case before someone asks, and it is not clear enough, this supports localized strings, so non-english speakers may search settings in their native language.

Looking at the code, the only nits I have are the use of string.find(x, ...) instead of x:find(...), and probably how the elseif block at line 372 is split. Both of those are minor issues for me and it's fine either way.

@Wuzzy2

This comment has been minimized.

Show comment
Hide comment
@Wuzzy2

Wuzzy2 Nov 26, 2016

Contributor

Does it support pressing enter?

Contributor

Wuzzy2 commented Nov 26, 2016

Does it support pressing enter?

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy
Member

rubenwardy commented Nov 26, 2016

Yes

@Fixer-007

This comment has been minimized.

Show comment
Hide comment
@Fixer-007

Fixer-007 Nov 27, 2016

Contributor

This is much needed, advanced settings are insanely complex.

Contributor

Fixer-007 commented Nov 27, 2016

This is much needed, advanced settings are insanely complex.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Nov 27, 2016

Contributor

From reading the code it seems that pressing enter multiple times doesn't neccesarily go through the list of matching search results, right? That should be added IMO.

Contributor

est31 commented Nov 27, 2016

From reading the code it seems that pressing enter multiple times doesn't neccesarily go through the list of matching search results, right? That should be added IMO.

@sfan5 sfan5 removed this from the 0.4.15 milestone Dec 7, 2016

@octacian

This comment has been minimized.

Show comment
Hide comment
@octacian

octacian Dec 9, 2016

Contributor

So.. any reason why this hasn't yet been merged? Seems to work alright to me, need to test again though. This is definitely something MT really needs though, in the server list too. +1

Contributor

octacian commented Dec 9, 2016

So.. any reason why this hasn't yet been merged? Seems to work alright to me, need to test again though. This is definitely something MT really needs though, in the server list too. +1

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Dec 9, 2016

Member

@octacian

it's called FEATURE FREEZE

Member

sfan5 commented Dec 9, 2016

@octacian

it's called FEATURE FREEZE

@octacian

This comment has been minimized.

Show comment
Hide comment
@octacian

octacian Dec 9, 2016

Contributor

@sfan5 right, I forgot about the freeze. I doubt I'll actually be really used to it till at least the 0.4.16 freeze :rotfl:

Contributor

octacian commented Dec 9, 2016

@sfan5 right, I forgot about the freeze. I doubt I'll actually be really used to it till at least the 0.4.16 freeze :rotfl:

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Dec 27, 2016

Member

Rebased

From reading the code it seems that pressing enter multiple times doesn't neccesarily go through the list of matching search results, right? That should be added IMO.

Added as separate commit to make it easier to review. It should be squashed when merging

Member

rubenwardy commented Dec 27, 2016

Rebased

From reading the code it seems that pressing enter multiple times doesn't neccesarily go through the list of matching search results, right? That should be added IMO.

Added as separate commit to make it easier to review. It should be squashed when merging

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Dec 27, 2016

Member

This PR removes the ability to use enter to open/edit a setting, unfortunately. I'm not sure how you'd fix that

Member

rubenwardy commented Dec 27, 2016

This PR removes the ability to use enter to open/edit a setting, unfortunately. I'm not sure how you'd fix that

@DS-Minetest

This comment has been minimized.

Show comment
Hide comment
@DS-Minetest

DS-Minetest Dec 28, 2016

Contributor

i dont think, its a big problem to lose the ability to open a setting with enter (i didnt even know that it is so)
the most people control the settings with mouse, not with keyboard since you have to use everywhere else in the main menu the mouse , there would be too many settings to scroll through and you cant open the yellow setting tabs (with the + at the front) with enter

Contributor

DS-Minetest commented Dec 28, 2016

i dont think, its a big problem to lose the ability to open a setting with enter (i didnt even know that it is so)
the most people control the settings with mouse, not with keyboard since you have to use everywhere else in the main menu the mouse , there would be too many settings to scroll through and you cant open the yellow setting tabs (with the + at the front) with enter

@SmallJoker SmallJoker referenced this pull request Jan 6, 2017

Closed

Search for settings #5001

@MarkuBu

This comment has been minimized.

Show comment
Hide comment
@MarkuBu

MarkuBu Jan 6, 2017

Contributor

Does search only works for localized strings or also for the technical names? Sometimes I don't know how it is translated and it is easier to search for the original text

Contributor

MarkuBu commented Jan 6, 2017

Does search only works for localized strings or also for the technical names? Sometimes I don't know how it is translated and it is easier to search for the original text

@DS-Minetest

This comment has been minimized.

Show comment
Hide comment
@DS-Minetest

DS-Minetest Jan 6, 2017

Contributor

This has no WIP label, FEATURE FREEZE is over and the feature would be very useful.
So, why isn't this merged?

Contributor

DS-Minetest commented Jan 6, 2017

This has no WIP label, FEATURE FREEZE is over and the feature would be very useful.
So, why isn't this merged?

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jan 6, 2017

Member

This has no WIP label, FEATURE FREEZE is over and the feature would be very useful.
So, why isn't this merged?

Because no other core devs have reviewed it

also for the technical names

yes. It searches in technical name, readable name, and description

Member

rubenwardy commented Jan 6, 2017

This has no WIP label, FEATURE FREEZE is over and the feature would be very useful.
So, why isn't this merged?

Because no other core devs have reviewed it

also for the technical names

yes. It searches in technical name, readable name, and description

@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Jan 7, 2017

Contributor

👍 from me for concept. If a person (even if they're not a core dev, but one of the long-term contributors) can review the Lua then I think this should be merged.

Contributor

Zeno- commented Jan 7, 2017

👍 from me for concept. If a person (even if they're not a core dev, but one of the long-term contributors) can review the Lua then I think this should be merged.

@Wuzzy2

This comment has been minimized.

Show comment
Hide comment
@Wuzzy2

Wuzzy2 Jan 8, 2017

Contributor

Looks good for me.
I would make another enter press when the selected entry is the last one make select the first entry again, so that the selection goes merry-go-round. ;-)

Contributor

Wuzzy2 commented Jan 8, 2017

Looks good for me.
I would make another enter press when the selected entry is the last one make select the first entry again, so that the selection goes merry-go-round. ;-)

@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jan 16, 2017

Member

Now a keyword based search is used (like the server search) and the best result is auto selected, rather than the first

Member

rubenwardy commented Jan 16, 2017

Now a keyword based search is used (like the server search) and the best result is auto selected, rather than the first

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Jan 17, 2017

Member

👍

code is simple, works as intended. I think this is ready for merge (for a while now).

Member

sofar commented Jan 17, 2017

👍

code is simple, works as intended. I think this is ready for merge (for a while now).

@sofar sofar added the One approval label Jan 17, 2017

@nerzhul

okay for me too

@nerzhul nerzhul merged commit a378e32 into minetest:master Jan 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rubenwardy rubenwardy deleted the rubenwardy:settings-search branch Jan 18, 2017

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