[TIMOB-26655] fix(ios): fix application shortcuts parity #10539

hansemannn · 2018-12-14T13:16:50Z

JIRA: https://jira.appcelerator.org/browse/TIMOB-26655

Please add no tests, since this is a UI related change that requires explicit user interaction.

build · 2018-12-14T13:42:54Z

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖	✅ All tests are passing Nice one! All 2990 tests are passing.

Generated by 🚫 dangerJS against c9ee5c0

janvennemann

Just some notes about the mutating enumerations and param naming. cc @vijaysingh-axway

janvennemann · 2018-12-17T15:44:26Z

iphone/Classes/TiUIApplicationShortcutsProxy.m

-      [shortcuts removeObject:item];
+  [shortcuts enumerateObjectsUsingBlock:^(UIApplicationShortcutItem * _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
+    if ([obj.type isEqualToString:key]) {
+      [shortcuts removeObject:obj];


It's not a good idea to mutate an array while enumerating it. Consider storing the indices that need to be removed in a NSMutableIndexSet and then remove them using removeObjectsAtIndexes. I know it was already wrong in the first place but since you are touching it here anyway let's make it right ;)

Also any particular reason you switched to enumerating using block instead of for (... in ...). I don't see any benefit but instead it makes it more verbose.

Damn, you're right with the mutation, I oversaw that! Regarding the enumerateObjectsUsingBlock : It's the faster and more ObjC'ish way of iterating. Some inspiring words can be found here.

janvennemann · 2018-12-17T15:45:08Z

iphone/Classes/TiUIiOSApplicationShortcutsProxy.m

-      [shortcuts removeObject:item];
+  [shortcuts enumerateObjectsUsingBlock:^(UIApplicationShortcutItem * _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
+    if ([obj.type isEqualToString:key]) {
+      [shortcuts removeObject:obj];


Same as above, do not mutate the array while enumerating it.

janvennemann · 2018-12-17T15:57:20Z

iphone/Classes/TiUIApplicationShortcutsProxy.m

-        [args objectForKey:@"identifier"]);
-    return;
-  }
-
  UIApplicationShortcutItem *shortcut = [[[UIApplicationShortcutItem alloc] initWithType:[args objectForKey:@"identifier"]


Does it make sense to rename this to itemtype too? Not sure why it's named identifier here and itemtype in the other place.

SDK 7.5.0 renamed it to identifier for the new API on the Ti.UI namespace which uses identifier for better parity with Android. I don't exactly remember why the old one wasn't just used here as well.

janvennemann · 2018-12-17T16:14:35Z

@hansemannn please fix the linting errors too, thanks!

hansemannn · 2018-12-17T16:21:53Z

@janvennemann PR updated. EDIT: Linting still fails, but clang-format -style=file -i Classes/TiUIApplicationShortcutsProxy.m attempts to use a different code-style. Did that change recently?

vijaysingh-axway · 2018-12-18T04:45:16Z

@hansemannn
If we are not making 'identifier' unique, there are following issues can arise -

In -(void)see getDynamicShortcut:(id)identifier, it always compare identifier and return the shortcut. If identifier is not unique we will always get first item.
In -(void)removeDynamicShortcut:(id)identifier, it will remove all shortcuts with this identifier. There will not be a way to remove a particular shortcut.

I think we should use ‘identifier’ (unique string to identify the shortcut) and ‘itemtype’ (app-defined type). Backward compatibility is also we have to take care .
I am not sure how android use ‘identifier’. Is it simple string to identify the shortcut uniquely or app-defined type as iOS?

hansemannn · 2019-01-07T12:55:27Z

@vijaysingh-axway Going back to this one, can you provide an example on how that API (identifier and itemtype) would look like? This is the only thing left blocking us from 8.0.0, so I'm optimistic!

hansemannn · 2019-01-07T14:11:13Z

I found a better way around it by actually making the identifiers unique. By using a prefix like OpenDetails_, I can use the ID of the model to make them unique + by iterating over that prefix, I can find the shortcuts to remove.

Still, I would keep the doc-changes as those have been a regression in 7.5+.

hansemannn · 2019-01-17T19:43:42Z

@mukherjee2 @vijaysingh-axway Can this please be integrated into 8.0.0? We're patching the SDK with this change since 7.4.1 and it's getting messy to update to newer SDK's without this being included.

Regarding the sensibility of this change: The fix is isolated to one SDK component and does not influence others. Also, it's more of a objc language-specific change than a change to the native layer or proxy. It is always reproducible and can easily be tested. If you need more test plans, we can provide those asap. Thank you.

One note regarding the removal of the identifier key: The docs already stated that the itemtype property is used in the shortcutitemclick, but until now, it used identifier on the Simulator (because all USE_TI_ statements are defined in the Simulator) and itemtype on the device. This PR fixes that behavior.

EDIT: Also added a backport in #10631 in case this receives traction.

vijaysingh-axway

CR passed.

lokeshchdhry · 2019-01-23T19:37:33Z

FR Passed.

Studio Ver: 5.1.2.201812191831
SDK Ver: 8.0.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.10-master.5
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.7
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2

build added this to the 8.0.0 milestone Dec 14, 2018

build requested review from a team December 14, 2018 13:42

build added community 🔥 docs ios needs tests 🚨 labels Dec 14, 2018

janvennemann requested changes Dec 17, 2018

View reviewed changes

hansemannn changed the title ~~[TIMOB-26655] fix(ios): make shorcut-item identifier non-unique, fix parity~~ [TIMOB-26655] fix(ios): fix application shortcuts parity Jan 7, 2019

build added needs tests 🚨 and removed needs tests 🚨 labels Jan 7, 2019

mukherjee2 modified the milestones: 8.0.0, 8.0.1 Jan 16, 2019

fix(ios): properly remove dynamic shortcuts

e524216

hansemannn force-pushed the TIMOB-26655 branch from c0f2a53 to e524216 Compare January 17, 2019 19:48

hansemannn mentioned this pull request Jan 17, 2019

[TIMOB-26655] (8_0_X) fix(ios): fix application shortcuts parity #10631

Merged

fix(ios): use itemtype for shortcuts

a6d5568

hansemannn force-pushed the TIMOB-26655 branch from 7658e15 to a6d5568 Compare January 18, 2019 12:40

Merge branch 'master' into TIMOB-26655

5f4091a

sgtcoolguy modified the milestones: 8.0.1, 8.1.0 Jan 23, 2019

Merge branch 'master' into TIMOB-26655

57470d2

vijaysingh-axway added the no tests label Jan 23, 2019

vijaysingh-axway removed the needs tests 🚨 label Jan 23, 2019

vijaysingh-axway approved these changes Jan 23, 2019

View reviewed changes

Merge branch 'master' into TIMOB-26655

c9ee5c0

lokeshchdhry merged commit bd46b84 into tidev:master Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-26655] fix(ios): fix application shortcuts parity #10539

[TIMOB-26655] fix(ios): fix application shortcuts parity #10539

hansemannn commented Dec 14, 2018 •

edited

build commented Dec 14, 2018 •

edited

janvennemann left a comment

janvennemann Dec 17, 2018

hansemannn Dec 17, 2018

janvennemann Dec 17, 2018

janvennemann Dec 17, 2018

hansemannn Dec 17, 2018

janvennemann commented Dec 17, 2018

hansemannn commented Dec 17, 2018 •

edited

vijaysingh-axway commented Dec 18, 2018

hansemannn commented Jan 7, 2019

hansemannn commented Jan 7, 2019

hansemannn commented Jan 17, 2019 •

edited

vijaysingh-axway left a comment

lokeshchdhry commented Jan 23, 2019

[TIMOB-26655] fix(ios): fix application shortcuts parity #10539

[TIMOB-26655] fix(ios): fix application shortcuts parity #10539

Conversation

hansemannn commented Dec 14, 2018 • edited

build commented Dec 14, 2018 • edited

janvennemann left a comment

Choose a reason for hiding this comment

janvennemann Dec 17, 2018

Choose a reason for hiding this comment

hansemannn Dec 17, 2018

Choose a reason for hiding this comment

janvennemann Dec 17, 2018

Choose a reason for hiding this comment

janvennemann Dec 17, 2018

Choose a reason for hiding this comment

hansemannn Dec 17, 2018

Choose a reason for hiding this comment

janvennemann commented Dec 17, 2018

hansemannn commented Dec 17, 2018 • edited

vijaysingh-axway commented Dec 18, 2018

hansemannn commented Jan 7, 2019

hansemannn commented Jan 7, 2019

hansemannn commented Jan 17, 2019 • edited

vijaysingh-axway left a comment

Choose a reason for hiding this comment

lokeshchdhry commented Jan 23, 2019

hansemannn commented Dec 14, 2018 •

edited

build commented Dec 14, 2018 •

edited

hansemannn commented Dec 17, 2018 •

edited

hansemannn commented Jan 17, 2019 •

edited