fix: support adding and removing Localizable.strings file #356
Conversation
XCode project supports adding Localizable.strings file in multiple variants in which each language gets its own variant with the language code as the file "name" property. Before ====== - The file would be added under incorrect Resources section - The file ID would still be referenced in the Localizable.strings PBXVariantGroup section event though it has been removed - File cannot have a custom name After ===== - The file can be added to the correct group via the `parent` parameter in ProjectFiles.add_file() method - When a file is removed, it's references are removed from both PBXGroup and PBXVariant group sections - Files can have custom names, differing from its path - More fixes for the `productRef` issue Fun fact ======== This code is written on Linux, with help from my colleagues to debug the version once it's ready on their iOS devices. Thanks! ======= This project has been very helpful for us in openedx/openedx-app-ios. Thanks for creating it and double thanks for your fast triage for the other pull request.
|
@kronenthaler Thanks for the help on the other pull request. Would you mind enabling tests for "Second time contributors" like myself? This can be done by selecting the "Require approval for first-time contributors" option as explained here: |
|
Thanks @kronenthaler! I would appreciate a quick review before I go on and add tests for the new features I've built. I really appreciate your help! I'm working on another opensource project and your help by both providing this project and traging pull requests has been remarkable. |
| def add_file(self, path, name=None, parent=None, tree=TreeType.SOURCE_ROOT, target_name=None, force=True, | ||
| file_options=FileOptions()): |
There was a problem hiding this comment.
@kronenthaler I was wondering if name should be added to FileOptions instead?
Let me know, I'm happy to refactor it this way.
I suppose this way we'll be much more backward compatible.
There was a problem hiding this comment.
I will need to take a deeper look. At first glance, it should not break backwards compatibility given that provides a default value (and its management is consistent)
The deeper question i have is why is this necessary? Xcode infers the name of the file from the path, and that is what it adds to the project.
There was a problem hiding this comment.
@kronenthaler I need to take a deeper look as well myself 😅, I developed this on a Linux machine. It's like developing with your eyes closed.
I sent the original picture to my colleague (in another country) so he test and I found the following:
You can see the conversation here: openedx/openedx-app-ios#465 (comment)
So after a couple of attempts I noticed that XCode won't detect the translation files unless they're in the PBXVariantGroup and the name of the would be set to the locale instead of the path. Therefore, I chose to override the name and make it different.
My question, does it make sense to add the name in FileOptions? to ensure much more robust backward compatibility or the function parameter makes more sense?
| @@ -199,7 +200,7 @@ def _filter_targets_without_path(self, path, target_name): | |||
| build_phase = self.get_object(build_phase_id) | |||
| for build_file_id in build_phase.files: | |||
| build_file = self.get_object(build_file_id) | |||
| if build_file is None: | |||
| if build_file is None or not hasattr(build_file, 'fileRef'): | |||
There was a problem hiding this comment.
More productRef fixes.
| for group in filter(lambda x: file_ref.get_id() in x.children, | ||
| self.objects.get_objects_in_section('PBXGroup', 'PBXVariantGroup')): |
There was a problem hiding this comment.
Since .strings go into PBXVariantGroup, do we need to add a new remove_file_by_id method, or perhaps add a remove_file_by_id(..., section='PBXGroup') parameter?
I haven't decided which one's the best way, so I'm happy to hear.
There was a problem hiding this comment.
Related to Variant groups, there is an unmerged branch about adding support for it. The thing is, variant groups are not trivial, multiple operations need to be carried out in order for xcode to deal with the strings properly.
There was a problem hiding this comment.
@kronenthaler You're right, the code in this pull request requires the user (the other developers) to know what they're doing. It's not possible "just" add a Localizable.strings, but the multiple operations needs to be called.
Would you mind sharing the branch so we can move it forward? I don't completely understand this project so I'm depending a lot on trial and error and relying on unit tests to cut my way through. If there's a more designed solution, I'm happy to help.
There was a problem hiding this comment.
@kronenthaler Hi! Thanks for your help and review so far.
I would like to move forward with the Localizable.strings support in this repository and I wanted to know what's your plan? We can go with either of the following direction:
- Add the bare minimum changes that doesn't break this package and keep the rest in our openedx/openedx-app-ios repository scripts.
- Add full support for the
Localizable.stringsandPBXVariantGroupso the users like openedx/openedx-app-ios have a clean interface likeadd_file_variant(base="Localizable.strings", variant_name="uk", path="Profile/uk.lproj/Localizable.strings")andremove_file_variant(base="Localizable.strings", variant_name="uk", path="Profile/uk.lproj/Localizable.strings")orremove_file_variants(base="Localizable.strings")
There was a problem hiding this comment.
I have just checked the related work so far like https://github.com/kronenthaler/mod-pbxproj/pull/124/files and master...variant-groups and I'm looking forward to learn more about them once you think they're ready. From what I see you're going with the cleaner approach of providing an API that fully supports the PBXVariantGroup which is great.
I hate to admit this again, but I'm developing this on a Linux machine and it's my first encounter of pbx project files which doesn't help me to get the most suitable solution.
| from pbxproj.pbxsections import PBXFileReference, PBXGroup | ||
|
|
||
|
|
||
| class PBXVariantGroup(PBXGroup): |
There was a problem hiding this comment.
I have added this class and everything works just magically. I have no idea how the parser works. Super magic @kronenthaler!
Where's the actual string parser code though?
There was a problem hiding this comment.
Neat!
@classmethod
def _get_class_reference(cls, class_type):
module = __import__('pbxproj')
return getattr(module, class_type, PBXGenericObject)
| @@ -637,7 +639,7 @@ def _create_build_products(self, product_ref, target_name): | |||
|
|
|||
| @classmethod | |||
| def _determine_file_type(cls, file_ref, unknown_type_allowed): | |||
| ext = os.path.splitext(file_ref.get_name())[1] | |||
| ext = os.path.splitext(file_ref.path)[1] | |||
There was a problem hiding this comment.
Name is customizable, so it's better to use path which is more explicit.
Co-authored-by: Ignacio Calderon <kronenthaler@users.noreply.github.com>
Co-authored-by: Ignacio Calderon <kronenthaler@users.noreply.github.com>
|
@kronenthaler Would you mind sharing an update on how do we think we should proceed with this pull request? |
I will take a look somewhere this week and advise how to proceed. |
XCode project supports adding Localizable.strings file in multiple variants in which each language gets its own variant with the language code as the file "name" property.
Before
After
parentparameter in ProjectFiles.add_file() methodproductRefissueTODO
Fun fact
This code is written on Linux, with help from my colleagues to debug the version once it's ready on their iOS devices.
Thanks!
This project has been very helpful for us in openedx/openedx-app-ios. Thanks for creating it and double thanks for your fast triage for the other pull request.