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.

Sign up for GitHub

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

fix: support adding and removing Localizable.strings file #356

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

fix: support adding and removing Localizable.strings file #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd6eca6
fix: support adding and removing Localizable.strings file
OmarIthawi Jun 30, 2024
a9187b4
fix: fixup previous commit
OmarIthawi Jun 30, 2024
16a373c
Update pbxproj/pbxextensions/ProjectFiles.py
OmarIthawi Jul 3, 2024
be404e5
Update pbxproj/pbxextensions/ProjectFiles.py
OmarIthawi Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions pbxproj/pbxextensions/ProjectFiles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,15 @@ class ProjectFiles:
def __init__(self):
raise EnvironmentError('This class cannot be instantiated directly, use XcodeProject instead')

def add_file(self, path, parent=None, tree=TreeType.SOURCE_ROOT, target_name=None, force=True,
def add_file(self, path, name=None, parent=None, tree=TreeType.SOURCE_ROOT, target_name=None, force=True,
file_options=FileOptions()):
Comment on lines +148 to 149
Copy link
Contributor Author

@OmarIthawi OmarIthawi Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

image

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?

"""
Adds a file to the project, taking care of the type of the file and creating additional structures depending on
the file type. For instance, frameworks will be linked, embedded and search paths will be adjusted automatically.
Header file will be added to the headers sections, but not compiled, whereas the source files will be added to
the compilation phase.
:param path: Path to the file to be added
:param name: Optional custom name of the file, if None is passed then "path" will be used.
:param parent: Parent group to be added under
:param tree: Tree where the path is relative to
:param target_name: Target name or list of target names where the file should be added (none for every target)
Expand All @@ -167,7 +168,7 @@ def add_file(self, path, parent=None, tree=TreeType.SOURCE_ROOT, target_name=Non
if target_name.__len__() == 0:
return []

file_ref, abs_path, path, tree, expected_build_phase = self._add_file_reference(path, parent, tree, force,
file_ref, abs_path, path, tree, expected_build_phase = self._add_file_reference(path, name, parent, tree, force,
file_options)
if path is None or tree is None:
return None
Expand Down Expand Up @@ -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'):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More productRef fixes.

continue

file_ref = self.get_object(build_file.fileRef)
Expand Down Expand Up @@ -236,7 +237,7 @@ def add_project(self, path, parent=None, tree=TreeType.GROUP, target_name=None,
if not force and self._file_exists(path):
return []

file_ref, _, path, tree, expected_build_phase = self._add_file_reference(path, parent, tree, force,
file_ref, _, path, tree, expected_build_phase = self._add_file_reference(path, None, parent, tree, force,
file_options)
if path is None or tree is None:
return None
Expand Down Expand Up @@ -360,7 +361,8 @@ def remove_file_by_id(self, file_id, target_name=None):
return True

# remove the file from any groups if there is no reference from any target
for group in filter(lambda x: file_ref.get_id() in x.children, self.objects.get_objects_in_section('PBXGroup')):
for group in filter(lambda x: file_ref.get_id() in x.children,
self.objects.get_objects_in_section('PBXGroup', 'PBXVariantGroup')):
Comment on lines +364 to +365
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@OmarIthawi OmarIthawi Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.strings and PBXVariantGroup so the users like openedx/openedx-app-ios have a clean interface like add_file_variant(base="Localizable.strings", variant_name="uk", path="Profile/uk.lproj/Localizable.strings") and remove_file_variant(base="Localizable.strings", variant_name="uk", path="Profile/uk.lproj/Localizable.strings") or remove_file_variants(base="Localizable.strings")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

group.remove_child(file_ref)

# the file is not referenced in any build file, remove it
Expand Down Expand Up @@ -412,8 +414,8 @@ def add_folder(self, path, parent=None, excludes=None, recursive=True, create_gr
# add the top folder as a group, make it the new parent
path = os.path.abspath(path)
if not create_groups and os.path.splitext(path)[1] not in ProjectFiles._SPECIAL_FOLDERS:
return self.add_file(path, parent, target_name=target_name, force=False, tree=TreeType.GROUP,
file_options=file_options)
return self.add_file(path, parent=parent, target_name=target_name,
force=False, tree=TreeType.GROUP, file_options=file_options)

parent = self.get_or_create_group(os.path.split(path)[1], path, parent, make_relative=file_options.add_groups_relative)

Expand All @@ -428,8 +430,8 @@ def add_folder(self, path, parent=None, excludes=None, recursive=True, create_gr
if os.path.isfile(full_path) or os.path.splitext(child)[1] in ProjectFiles._SPECIAL_FOLDERS or \
not create_groups:
# check if the file exists already, if not add it
children = self.add_file(full_path, parent, target_name=target_name, force=False, tree=TreeType.GROUP,
file_options=file_options)
children = self.add_file(full_path, parent=parent, target_name=target_name,
force=False, tree=TreeType.GROUP, file_options=file_options)
else:
# if recursive is true, go deeper, otherwise create the group here.
if recursive:
Expand Down Expand Up @@ -572,14 +574,14 @@ def add_package_dependency(self, target_name, create_parameters=()):

# miscellaneous functions, candidates to be extracted and decouple implementation

def _add_file_reference(self, path, parent, tree, force, file_options):
def _add_file_reference(self, path, name, parent, tree, force, file_options):
# decide the proper tree and path to add
abs_path, path, tree = ProjectFiles._get_path_and_tree(self._source_root, path, tree)
if path is None or tree is None:
return None, abs_path, path, tree, None

# create a PBXFileReference for the new file
file_ref = PBXFileReference.create(path, tree)
file_ref = PBXFileReference.create(path, name, tree)

# determine the type of the new file:
file_type, expected_build_phase = ProjectFiles._determine_file_type(file_ref, file_options.ignore_unknown_type)
Expand Down Expand Up @@ -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]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is customizable, so it's better to use path which is more explicit.

if os.path.isdir(os.path.abspath(file_ref.path)) and ext not in ProjectFiles._SPECIAL_FOLDERS:
file_type = 'folder'
build_phase = 'PBXResourcesBuildPhase'
Expand Down
8 changes: 4 additions & 4 deletions pbxproj/pbxextensions/ProjectGroups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def remove_group_by_id(self, group_id, recursive=True):
del self.objects[group.get_id()]

# remove the reference from any other group object that could be containing it.
for other_group in self.objects.get_objects_in_section('PBXGroup'):
for other_group in self.objects.get_objects_in_section('PBXGroup', 'PBXVariantGroup'):
other_group.remove_child(group)

return True
Expand All @@ -87,14 +87,14 @@ def remove_group_by_name(self, group_name, recursive=True):

return True

def get_groups_by_name(self, name, parent=None):
def get_groups_by_name(self, name, parent=None, section='PBXGroup'):
"""
Retrieve all groups matching the given name and optionally filtered by the given parent node.
:param name: The name of the group that has to be returned
:param parent: A PBXGroup object where the object has to be retrieved from. If None all matching groups are returned
:return: An list of all matching groups
"""
groups = self.objects.get_objects_in_section('PBXGroup')
groups = self.objects.get_objects_in_section(section)
groups = [group for group in groups if group.get_name() == name]

if parent:
Expand All @@ -110,7 +110,7 @@ def get_groups_by_path(self, path, parent=None):
:param parent: A PBXGroup object where the object has to be retrieved from. If None all matching groups are returned
:return: An list of all matching groups
"""
groups = self.objects.get_objects_in_section('PBXGroup')
groups = self.objects.get_objects_in_section('PBXGroup', 'PBXVariantGroup')
groups = [group for group in groups if group.get_path() == path]

if parent:
Expand Down
6 changes: 3 additions & 3 deletions pbxproj/pbxsections/PBXFileReference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@

class PBXFileReference(PBXGenericObject):
@classmethod
def create(cls, path, tree='SOURCE_ROOT'):
def create(cls, path, name=None, tree='SOURCE_ROOT'):
return cls().parse({
'_id': cls._generate_id(),
'isa': cls.__name__,
'path': path,
'name': os.path.split(path)[1],
'name': name or os.path.split(path)[1],
'sourceTree': tree
})

Expand Down Expand Up @@ -48,7 +48,7 @@ def remove(self, recursive=True):
build_file.remove(recursive)

# search for each group that has a reference to the build file and remove it from it.
for group in parent.get_objects_in_section('PBXGroup'):
for group in parent.get_objects_in_section('PBXGroup', 'PBXVariantGroup'):
if self.get_id() in group.children:
group.remove_child(self)

Expand Down
13 changes: 13 additions & 0 deletions pbxproj/pbxsections/PBXVariantGroup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from pbxproj.pbxsections import PBXFileReference, PBXGroup


class PBXVariantGroup(PBXGroup):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 parts on the parsing, one is the parsing of the openstep format to JSON like (that is here), and the second is mapping the JSON-like to objects, that's pretty much done here. Basically, creating instances of the object with the same class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

    @classmethod
    def _get_class_reference(cls, class_type):
        module = __import__('pbxproj')
        return getattr(module, class_type, PBXGenericObject)

def add_child(self, child):
# Note: PBXVariantGroup is typically used for Localizable.strings
# If other children type is needed e.g. PBXFileReference, edit this
# function.
if not isinstance(child, PBXFileReference):
return False

self.children.append(child.get_id())
return True
1 change: 1 addition & 0 deletions pbxproj/pbxsections/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from pbxproj.pbxsections.PBXLegacyTarget import *
from pbxproj.pbxsections.PBXReferenceProxy import *
from pbxproj.pbxsections.PBXGroup import *
from pbxproj.pbxsections.PBXVariantGroup import *
from pbxproj.pbxsections.XCSwiftPackageProductDependency import *
from pbxproj.pbxsections.XCRemoteSwiftPackageReference import *
from pbxproj.pbxsections.XCLocalSwiftPackageReference import *