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

Add ForEachString #388

Closed
wants to merge 2 commits into from
Closed

Add ForEachString #388

wants to merge 2 commits into from

Conversation

Ghabry
Copy link
Member

@Ghabry Ghabry commented Aug 25, 2020

This adds a new function ForEachString for replacing strings in LCF. Used for translation as discussed in that Player PR.

CC @sorlok

@Ghabry Ghabry added this to the 0.6.3 milestone Aug 25, 2020
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Aug 25, 2020

  1. I would change the lambda to not have a return value, then I can write functions which only inspect the strings.
[](auto& field, const auto& name) { field = transform(field); };
[](auto& field, const auto& name) { Utils::ToUpperInplace(field); };
[](const auto& field, const auto& name) { doSomethingReadOnly(field); };

This also grants some potential efficiency as you can modify the already allocated string object in place and not create a new one with a return value.

  1. Also, the name is not enough information. Inside my lambda, you give me the name of the field but I don't which struct it is. I would also pass in the struct itself. If we want the struct name as a string, we can add more reflection on the struct types themselves to add their names.
f(*this, this->name, "name");
  1. Consider making ForEachString() a free function overloaded for all rpg types instead of a member function. That makes it easier to extend to other types.

@Ghabry
Copy link
Member Author

Ghabry commented Aug 25, 2020

What is "a free function overloaded for all rpg types". (I only know overloading in the context of class inheritance)

Do you mean a template function with specialization per rpg type?

@fmatthew5876
Copy link
Contributor

template <typename F>
void ForEachString(lcf::rpg::SaveActor&, const F& f);
template <typename F>
void ForEachString(lcf::rpg::SaveMapEvent&, const F& f);

etc..

Don't template the struct type, C++ already gives us function overloading to do that.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 1, 2020

Another use case: Reencoding the entire database without parsing again (at least once, for "Unknown -> Detected")

@sorlok
Copy link

sorlok commented Sep 3, 2020

So right now you get the property's name ("end_game") and the value ("Shutdown"). When looking this up in the .po files, we actually look up by context ("term"). See sample code below.

	// Translate all terms
	lcf::Data::terms.ForEachString([this](const std::string& value, const std::string& name)->std::string {
		if (sys!=nullptr) {
			return sys->translate_ctxt("term", value);
		}
		return value;
	});

I think the following changes make sense:

  1. Pass the context ("term") in to the callback function as well. This streamlines translation substitution.
  2. Switch the order of name and value (stylistic)
  3. Pass the value in by NON-const reference, so we don't have to always overwrite it.

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 4, 2020

Another use case: Reencoding the entire database without parsing again (at least once, for "Unknown -> Detected")

So you mean we don't have to load the entire database twice?? That'll be a huge startup time performance win for player!

@sorlok
Copy link

sorlok commented Sep 4, 2020

One other thing to consider (in addition to my comment above) ---right now, if you switch from Default language to Spanish, then from Spanish to French, I have to reload the database (so that we can look up strings properly). I don't think this is worth fixing, since switching between multiple languages at runtime is an edge use case. Just to make sure everyone's aware though.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 4, 2020

So you mean we don't have to load the entire database twice?? That'll be a huge startup time performance win for player!

Yeah exactly this. See #169

So right now you get the property's name ("end_game") and the value ("Shutdown"). When looking this up in the .po files, we actually look up by context ("term"). See sample code below.

matthew already suggested to also provide an object instance. You can use the object instance to get a name through "reflection": Struct<std::remove_reference_t<decltype(instance)>>::name. But this is 1. not public API and 2. uglyyyy. Maybe add some lcf/reflection.h header which abstracts this stuff (GetTypename(rpg::instance&), also add ForEachString there), then we can freely alter the templates later without breaking code.
CC @fmatthew5876 You are the template master here (I needed 10 minutes to make this Struct line compile ;)). thoughts?

The "name" it returns in your case would be "Terms". So I must update lcftrans to use these names then. But when it is in liblcf they can't go out-of-sync :)

@sorlok
Copy link

sorlok commented Sep 4, 2020

The "reflection" idea sounds fine to me. I also like that LcfTrans can be updated to always be in sync on these contexts.

Is there any case where the name of the "context" (passed in to getText()) would need to be very different from the name of the class? That's the only thing I can think of that might trip this up. So far, I can't think of any case where this would be true.

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 4, 2020

Struct<std::remove_reference_t<decltype(instance)>>::name

Reader struct is not part of the public API, and it never should be.

What I think we need here is a new rpg::reflect mechanism that is public, and refactor reader_struct to use it.

@sorlok
Copy link

sorlok commented Sep 8, 2020

Hi, just checking in. Should I wait for the refactoring to ForEachString() or continue coding with the current API? (This is for the Translation PR I'm working on.) I've still got lots of side stuff to do, so no rush.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 9, 2020

closing in favor of #394

@Ghabry Ghabry closed this Sep 9, 2020
@Ghabry
Copy link
Member Author

Ghabry commented Sep 9, 2020

@sorlok

Please wait a bit more. I will ping you when it is ready.

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

Successfully merging this pull request may close these issues.

None yet

3 participants