Skip to content
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

Campaign selection: Add search functionality #3847

Closed
wants to merge 9 commits into from

Conversation

@dvladf
Copy link
Contributor

commented Jan 6, 2019

Added search functionality for campaign selection (issue #3829).
screenshot_20190106_233133
Initially, I did as it was in the unit_recruit.cpp, but the search there is case-sensitive for Russian locale.

[text_box]
id = "filter_box"
definition = "default"
{FITER_TEXT_BOX_HINT}

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 6, 2019

Member

Typo: "FILTER_TEXT_BOX_HINT".

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 7, 2019

Author Contributor

It doesn't work, because macro don't defined. In the logs when starting : error preprocessor: Macro/file 'FILTER_TEXT_BOX_HINT' is missing. Everywhere uses the FITER_TEXT_BOX_HINT. Can I fix this in the current pull request?

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 7, 2019

Member

Wow. I implemented the search functionality in the recruit screen and didn't even notice this.

Yes, in this pull request will be fine. Thank you for fixing it. Please make it a separate commit for reviewing and cherry-picking purposes.

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 7, 2019

Author Contributor

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 7, 2019

Member

Thank you. I wonder if we should we add a compatibility definition #define FITER_TEXT_BOX_HINT {FILTER_TEXT_BOX_HINT}#enddef? Does UMC use this macro?

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 8, 2019

Author Contributor

Hm, I don't thought about it. Probably, it's better to add a compatibility definition. But I'm not sure about this :)

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 8, 2019

Member

Let's just add a compatibility definition and move on.

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 8, 2019

Author Contributor

Okay, added it.

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 9, 2019

Member

Thanks. At this point I'm just waiting on understanding whether or not the gettext.cpp function should be using a mutex. If you find the answer in boost::locale documentation that would help (I already looked myself), or maybe one of the other developers will know the answer.

This comment has been minimized.

Copy link
@Vultraz

Vultraz Jan 17, 2019

Member

No compatibility macro is needed since this is for internal use only and not exposed to UMC. I've committed (05ab79f) a fix separately to keep this PR clean.


return std::search(ls1.begin(), ls1.end(),
ls2.begin(), ls2.end()) != ls1.end();
}

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 6, 2019

Member

I assume that unit_create.cpp and unit_recruit.cpp (#3787) should start using this version of ci_search? Also @rocketBANG's #3759 once it's merged?

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Jan 6, 2019

Contributor

the line above also seems to have wrong indentation.

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 7, 2019

Author Contributor

@jostephd
Yes, in these filters case-sensitive search for the Russian language.
For example, in the hotkeys preferences:
screenshot_20190107_081121
screenshot_20190107_081153

@gfgtdf Done

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 7, 2019

Member

Okay. I assume those filters should be case-insensitive for Russian. Could you fix those other places to use translation::ci_search instead of their own versions of ci_search?

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 7, 2019

Author Contributor

@jostephd jostephd referenced this pull request Jan 6, 2019

Merged

Filter hotkeys #3759

@jostephd

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

You implemented a substring search. Shouldn't this use a "all the words appear" search, like #3787 and #3759 do, so filtering for sout guar would find The South Guard?

When the campaign selection dialog open please set the focus to the filter textbox, but in such a way that pressing up/down still scrolls the campaigns list. This is how unit_recruit.cpp does it and it's convenient (it means you don't have to press the mouse first).

@jostephd
Copy link
Member

left a comment

Thank you for all the corrections! Reviewed and tested. Everything looks good, except that in gettext.cpp I don't understand why that function needs to acquire the mutex.

find_widget<tree_view_node>(&window, was_selected, false).select_node();
}

campaign_selected(window);

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 7, 2019

Member

Could you explain why this function call is needed? It seems as though it was needed before the search functionality too?

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 8, 2019

Author Contributor

This function call need for display the details of the selected campaign. Otherwise, the details of the campaign that was selected before applying the filter will be displayed.

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 8, 2019

Member

Thanks, I see the problem this is solving. What I really meant by my question is why the code before this PR doesn't call campaign_selected(). I think the answer to that is because the select_node() call a few lines above would call campaign_selected() anyway because of these lines:

/***** Setup campaign tree. *****/
tree_view& tree = find_widget<tree_view>(&window, "campaign_tree", false);
tree.set_selection_change_callback(std::bind(&campaign_selection::campaign_selected, this, std::ref(window)));

I suppose we should therefore call campaign_selected() only in an else branch of that if.

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 8, 2019

Author Contributor

Thank you for the detailed explanation. I fix it.

@@ -515,4 +515,17 @@ std::string strftime(const std::string& format, const std::tm* time)
return dummy.str();
}

bool ci_search(const std::string& s1, const std::string& s2)
{
std::lock_guard<std::mutex> lock(get_mutex());

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 7, 2019

Member

Why does this function need to acquire a mutex?

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 8, 2019

Author Contributor

I'm myself not sure that a mutex is needed here :) I found this commit and as I understand it, mutexes in the gettext.cpp are used in the case of we have actually 2 active threads (the loadingscreen). But translation::ci_search function don't used in load functions.

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 8, 2019

Member

The loading screen isn't the only multithreaded part of wesnoth. I think we need to understand what the mutex is for (its purpose isn't documented anywhere that I can see). This function calls get_manager().get_locale() and boost::locale::to_lower(). Do these function calls need to be guarded by the mutex, or not? @gfgtdf, do you know?

@jostephd jostephd self-assigned this Jan 9, 2019

@jostephd jostephd added this to the 1.14.6 milestone Jan 12, 2019

@@ -515,4 +515,15 @@ std::string strftime(const std::string& format, const std::tm* time)
return dummy.str();
}

bool ci_search(const std::string& s1, const std::string& s2)
{
const std::locale& locale = get_manager().get_locale();

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 12, 2019

Member

Okay, so we have our answer:

<gfgtdf> get_locale() call shodul be pretected by mutex, otherwise update_locale_internal() might be called by two differnt threads at the same time.

Could you add the mutex back, please? I'll open a separate issue for documenting the need for a mutex.

edit: opened #3868

This comment has been minimized.

Copy link
@dvladf

dvladf Jan 13, 2019

Author Contributor

Thank you very much! Added the mutex back.

This comment has been minimized.

Copy link
@jostephd

jostephd Jan 13, 2019

Member

Thank you for implementing all the feedback! I'll merge this in a few days if none of the other developers has any further comments.

Vultraz added a commit that referenced this pull request Jan 17, 2019

Vultraz added a commit that referenced this pull request Jan 17, 2019

@jostephd

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Could someone merge this please? I would but I'm short on time. I know we're in string freeze but this PR adds no new strings.

@Vultraz Vultraz self-assigned this Jan 20, 2019

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Who is this Richard that approved this pr?

@Vultraz

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

I have no idea... P_P @RichardB122 how did you get assigned this PR?

@Vultraz

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Merged manually. Thanks for this!

@Vultraz Vultraz closed this Jan 27, 2019

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Perhaps someone with ownership should file an Issue with GitHub? My guess is their DBMS messed up and he was actually approving something in another project.

nemaara added a commit to nemaara/wesnoth that referenced this pull request Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.