chore(ios): move application shortcut under Ti.UI.Shortcut to have parity #11697

vijaysingh-axway · 2020-05-09T00:09:29Z

https://jira.appcelerator.org/browse/TIMOB-26818

NOTE - Merge with PR #11759

…rity

build · 2020-05-09T00:25:15Z

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖	✅ All tests are passing Nice one! All 6810 tests are passing. (There are 700 skipped tests not included in that total)

Generated by 🚫 dangerJS against 53cac84

sgtcoolguy · 2020-06-10T13:16:50Z

Relates to #11759

iphone/Classes/TiUIShortcutProxy.h

sgtcoolguy

I think we definitely need some basic unit tests around this.

Probably need to take this: https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.shortcutitem.test.js

Add it to tests/Resources here and drop the .android filters on the suite/tests.

iphone/Classes/TiUIShortcutProxy.h

iphone/Classes/TiUIShortcutItemProxy.h

iphone/Classes/TiUIShortcutItemProxy.m

iphone/Classes/TiUIiOSProxy.m

vijaysingh-axway · 2020-06-11T00:42:11Z

I think we definitely need some basic unit tests around this.

Probably need to take this: https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.shortcutitem.test.js

Add it to tests/Resources here and drop the .android filters on the suite/tests.

I saw, Gary has written unit test case in his PR. I tried to run at my end , it is working fine. Once that PR get merge, I'll update that to run for iOS as well.

iphone/Classes/TiUIShortcutProxy.m

sgtcoolguy · 2020-06-11T14:02:48Z

iphone/Classes/TiUIShortcutProxy.h

+
+- (TiUIShortcutItemProxy *)getById:(NSString *)identifier;
+
+- (void)remove:(TiUIShortcutItemProxy *)shortcut;


cc @garymathews So this is me really super nit-picking. Personally I always do a double-take on any method that returns void. It offers no direct means of determining what happened, or if something happened. You need to usually poke around the object more to tell if what you wanted to happen actually did.

So in this case, we're really talking about a collection and simply adding/removing elements. I think it'd be useful to know if that actually happened. So I think perhaps in this case a boolean return type may make sense.

For example, what if I pass in a shortcut to remove that was never added in the first place? That should return false and do nothing. What if the collection was already empty, removeAll should return false. Add would likely always return true or stay void - any inability to add a shortcut should more likely be an Error thrown.

You guys don't need to change this, just something to consider. Languages/frameworks are very spotty for something like these operations where a lot return void (Obj-C NSMutableArray), some return booleans (Java List), JS returns the new length of the collection for Array. So it's not a clear consensus. (And in Java's case, add always returns true)

sgtcoolguy

@vijaysingh-axway Please copy the tests from @garymathews 's PR for Android (and make them also run on iOS).

vijaysingh-axway · 2020-06-17T17:56:01Z

@vijaysingh-axway Please copy the tests from @garymathews 's PR for Android (and make them also run on iOS).

@sgtcoolguy Added the tests.
'remove' test case has been updated a bit. Reason is that 'remove' test case get finished before 'add'. In that case shortcut count get mismatched. While resolving merge conflict 'remove' test case from this PR need to be taken.

sgtcoolguy

Just needs updated apidocs

iphone/Classes/TiUIShortcutItemProxy.h

iphone/Classes/TiUIShortcutItemProxy.m

iphone/Classes/TiUIShortcutItemProxy.h

iphone/Classes/TiUIShortcutItemProxy.m

# Conflicts: # apidoc/Titanium/UI/Shortcut.yml # apidoc/Titanium/UI/ShortcutItem.yml # tests/Resources/ti.ui.shortcut.addontest.js # tests/Resources/ti.ui.shortcutitem.test.js

vijaysingh-axway · 2020-07-14T00:16:31Z

@garymathews In android, As per [doc] (https://github.com/appcelerator/titanium_mobile/blob/a378d21e0e5e2343ea3617941226066d8b48321e/apidoc/Titanium/UI/Shortcut.yml#L53), callback of Ti.UI.Shortcut click event should give 'item' property which should be Ti.UI.ShortcutItem, which is expected behavior as well.
But in example code here, you are using 'e.id' instead of 'e.item.id' to log shortcut id. Is this documentation issue or android has implemented in this way?
cc @sgtcoolguy @jquick-axway .

garymathews · 2020-07-15T22:04:53Z

@garymathews In android, As per [doc] (

https://github.com/appcelerator/titanium_mobile/blob/a378d21e0e5e2343ea3617941226066d8b48321e/apidoc/Titanium/UI/Shortcut.yml#L53

), callback of Ti.UI.Shortcut click event should give 'item' property which should be Ti.UI.ShortcutItem, which is expected behavior as well.
But in example code here, you are using 'e.id' instead of 'e.item.id' to log shortcut id. Is this documentation issue or android has implemented in this way?
cc @sgtcoolguy @jquick-axway .

Good catch, I've amended this with #11823

apidoc/Titanium/UI/Shortcut.yml

sgtcoolguy · 2020-07-17T17:55:18Z

manually rebased, squashed down commits, combined with latest unit tests for API and merged to master

chore(ios): move application shortcut under Ti.UI.Shortcut to have pa…

2fc6f0a

…rity

vijaysingh-axway added hold for parity ✋ This PR should not be merged until an equivalent PR is ready for the other platform(s) improvement ios labels May 9, 2020

vijaysingh-axway requested a review from garymathews May 9, 2020 00:10

build added this to the 9.1.0 milestone May 9, 2020

build requested a review from a team May 9, 2020 00:25

build added the needs tests 🚨 label May 9, 2020

sgtcoolguy reviewed Jun 10, 2020

View reviewed changes

iphone/Classes/TiUIShortcutProxy.h Show resolved Hide resolved

sgtcoolguy suggested changes Jun 10, 2020

View reviewed changes

vijaysingh-axway added 3 commits June 10, 2020 14:30

fix(ios): addressed review comments

92525d9

fix(ios): event firing issue

f4914c0

Merge branch 'master' into TIMOB-26818

4eea117

docs(ios): added deprecation

9ae8746

build added the docs label Jun 11, 2020

sgtcoolguy reviewed Jun 11, 2020

View reviewed changes

iphone/Classes/TiUIShortcutProxy.m Outdated Show resolved Hide resolved

sgtcoolguy reviewed Jun 11, 2020

View reviewed changes

fix(ios): maintain order of shortcut item after replacing

daea98d

sgtcoolguy self-requested a review June 16, 2020 17:35

sgtcoolguy reviewed Jun 16, 2020

View reviewed changes

vijaysingh-axway added 2 commits June 16, 2020 10:42

Merge branch 'master' into TIMOB-26818

83f8fd7

test(ios): added unit test

698563d

build removed the needs tests 🚨 label Jun 16, 2020

Merge branch 'master' into TIMOB-26818

81055b8

vijaysingh-axway removed the hold for parity ✋ This PR should not be merged until an equivalent PR is ready for the other platform(s) label Jul 2, 2020

fix(ios): revert unnecessary commited changes

3aff954

vijaysingh-axway added 4 commits July 8, 2020 13:07

Merge branch 'master' into TIMOB-26818

999cc52

style: fixed linting issue

3f6d781

Merge branch 'master' into TIMOB-26818

36cb565

style: fixed linting issue

3718553

sgtcoolguy self-requested a review July 10, 2020 18:44

sgtcoolguy suggested changes Jul 10, 2020

View reviewed changes

iphone/Classes/TiUIShortcutItemProxy.h Show resolved Hide resolved

iphone/Classes/TiUIShortcutItemProxy.m Outdated Show resolved Hide resolved

iphone/Classes/TiUIShortcutItemProxy.h Show resolved Hide resolved

iphone/Classes/TiUIShortcutItemProxy.m Outdated Show resolved Hide resolved

docs: updated docs

607d23d

build added the merge conflicts 🚨 label Jul 13, 2020

vijaysingh-axway added 2 commits July 13, 2020 16:41

docs: updated docs

db3fbbc

Merge branch 'master' into TIMOB-26818

d98ec20

# Conflicts: # apidoc/Titanium/UI/Shortcut.yml # apidoc/Titanium/UI/ShortcutItem.yml # tests/Resources/ti.ui.shortcut.addontest.js # tests/Resources/ti.ui.shortcutitem.test.js

Merge branch 'master' into TIMOB-26818

7d53aca

build removed the merge conflicts 🚨 label Jul 15, 2020

vijaysingh-axway added 2 commits July 15, 2020 15:41

docs: fixed example code

af4ec99

Merge branch 'master' into TIMOB-26818

7a5cac3

sgtcoolguy reviewed Jul 17, 2020

View reviewed changes

apidoc/Titanium/UI/Shortcut.yml Outdated Show resolved Hide resolved

docs: addressed review comment

53cac84

sgtcoolguy closed this Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ios): move application shortcut under Ti.UI.Shortcut to have parity #11697

chore(ios): move application shortcut under Ti.UI.Shortcut to have parity #11697

vijaysingh-axway commented May 9, 2020 •

edited

build commented May 9, 2020 •

edited

sgtcoolguy commented Jun 10, 2020

sgtcoolguy left a comment

vijaysingh-axway commented Jun 11, 2020 •

edited

sgtcoolguy Jun 11, 2020

sgtcoolguy left a comment

vijaysingh-axway commented Jun 17, 2020

sgtcoolguy left a comment

vijaysingh-axway commented Jul 14, 2020

garymathews commented Jul 15, 2020

sgtcoolguy commented Jul 17, 2020


		- (TiUIShortcutItemProxy )getById:(NSString )identifier;

		- (void)remove:(TiUIShortcutItemProxy *)shortcut;

chore(ios): move application shortcut under Ti.UI.Shortcut to have parity #11697

chore(ios): move application shortcut under Ti.UI.Shortcut to have parity #11697

Conversation

vijaysingh-axway commented May 9, 2020 • edited

build commented May 9, 2020 • edited

sgtcoolguy commented Jun 10, 2020

sgtcoolguy left a comment

Choose a reason for hiding this comment

vijaysingh-axway commented Jun 11, 2020 • edited

sgtcoolguy Jun 11, 2020

Choose a reason for hiding this comment

sgtcoolguy left a comment

Choose a reason for hiding this comment

vijaysingh-axway commented Jun 17, 2020

sgtcoolguy left a comment

Choose a reason for hiding this comment

vijaysingh-axway commented Jul 14, 2020

garymathews commented Jul 15, 2020

sgtcoolguy commented Jul 17, 2020

vijaysingh-axway commented May 9, 2020 •

edited

build commented May 9, 2020 •

edited

vijaysingh-axway commented Jun 11, 2020 •

edited