search

B3nedikt / reword

Reword is a android library to update the texts of views when the apps texts have changed due to a language change or an update of the apps string resources.
Apache License 2.0
32 stars 7 forks source link

Don't use `view.id` as unique key #5

Closed bleeding182 closed 3 years ago

bleeding182 commented 3 years ago

ViewTransformManager uses view.id to store attributes after the inflation to determine string changes later.

This will lead to issues when views share the same id (and probably won't work either when they don't have an ID)

Since this information should be stored per view one could use view.setTag(MY_ID) instead to store the information on the view, independent of any IDs. This would also eliminate the need for the attributes map and allow for better garbage collection (along with the view it belongs to)

B3nedikt commented 3 years ago

@bleeding182 Yeah, that is definitely an issue. Actually, while not explicitly documented these ids are unique positive integer numbers generated at compile time as long as they exist, if they don't they are "-1" to indicate no ID. So, views set in XML with no ID will not get "reworded".

Using the tag is also not a good idea IMHO as the user may change the tag in code & tags are not guaranteed to be unique either.

What I am currently experimenting with for version 2 of this lib is, instead of trying to find a unique aspect of the view, to replace the views with a wrapper View before inflation, which simply knows which attributes are needed to determine string changes later. When calling reword I would then have to simply check if the view is a wrapper view and reword it using the ViewTransformer. This would allow to have the same API as now and would allow for better garbage collection. To not kill of appcompat behavior which replaces views as well, I will need to have a specific wrapper view for each view though making the API more complex...

I am happy for any suggestions on how to improve the lib though ;) If you have crashes feel free to post the logs here.

bleeding182 commented 3 years ago

Using the tag is also not a good idea IMHO as the user may change the tag in code & tags are not guaranteed to be unique either.

You can use setTag (int key, Object tag) which has been around forever. If you use your own R.id.some_id for the key it'll even be somewhat guaranteed that there won't be any issues with somebody using the same ID.

There won't be any need for uniqueness as you can get rid of that hashmap completely and just save the information along with the view.

B3nedikt commented 3 years ago

That sounds good, I look into it :)

Yeah since API 4, so that should definitely be no problem. I think a own id is required though, as the docs say "Keys identified as belonging to the Android framework or not associated with any package will cause an IllegalArgumentException to be thrown." I don't know if this is enforced though ;)

This should be in the release at the end of this week together with new releases of some of my other libs. Do you use this lib in a project of yours or just researching?

B3nedikt commented 3 years ago

@bleeding182 See here: https://github.com/B3nedikt/reword/pull/6

B3nedikt commented 3 years ago

Released with 1.1.1. Thanks for the input :)

bleeding182 commented 3 years ago

Do you use this lib in a project of yours or just researching?

@B3nedikt yep, I'm just researching on different solutions and was playing around with this lib a bit.

Glad I could help