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

Use option index as row tag value in SelectorViewController #2007

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rayx
Copy link

@rayx rayx commented Mar 24, 2020

When using PushRow with custom type, if two instances of the custom type have same string representation, it would crash the App due to "Duplicate tag" error (see #552). As the author commented in that issue, the preferred method is to modify the custom type's description computed property to generate different string for different instance. However, that's not always feasible, given that the same string is also used as the text shown to user (meaning: one can't dump everything that helps to differentiate two instances in the string). In those (rare) cases, I think it's better to show those options with same text rather than crash the App. As far as I can tell, the only thing prevents that behavior is the tags generated in the code. The commit fixes that by making sure tags are unique.

@rayx rayx force-pushed the selectorviewcontroller_fix branch from 8b04038 to 1f3dcdf Compare March 25, 2020 13:16
@rayx rayx changed the title Append option index to the tags of the rows in SelectorViewController Use option index as row tag value in SelectorViewController Mar 25, 2020
@rayx
Copy link
Author

rayx commented Apr 4, 2020

@mats-claassen Could you please take a look at the new patch? The CI build failed. But from what I can tell, it didn't seem to have anything to do with my change (I tested the change in my own repo). Thanks.

@mats-claassen
Copy link
Member

It seems the unit tests failed. If you select the Eureka target scheme you can run them and see why they failed

@rayx
Copy link
Author

rayx commented Apr 11, 2020

Thanks for the notification. I'm tied up with my project and won't be able to try it soon. Will look into it when I get a chance (perhaps in June).

@xmartlabs xmartlabs deleted a comment from Ashruss Apr 30, 2020
Copy link

@aPandeyisystango aPandeyisystango left a comment

Choose a reason for hiding this comment

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

When I am using this piece of code and get rid of duplicate tag error.

@rayx
Copy link
Author

rayx commented Sep 17, 2020

@aPandeyisystango I'm not sure what exact issue you ran into. I included the patch in one of my apps and it worked well. If you look at the code change, there is no reason why it could fail. Are you sure the issue you hit was the same issue the patch addressed? Anyway I haven't done iOS app development for a few months and probably won't do it any time soon, so I'm afraid I can't help on this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants