[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a view on top #9622

vijaysingh-axway · 2017-11-21T08:12:08Z

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

… view on top

…view on top

… view on top

build · 2017-11-21T08:34:25Z

	Fails
🚫	🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

	Messages
📖	💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

hansemannn

Left a few review comments that should be addressed before continuing.

hansemannn · 2017-11-21T15:59:45Z

iphone/Classes/TiUIListView.m

+    if ([proxy isKindOfClass:[TiWindowProxy class]]) {
+      searchControllerPresenter = [proxy windowHoldingController];
+    } else {
+      searchControllerPresenter = [[TiApp app] controller];


Did you ensure this does not get released before closing the window?

This is not needed it is just used to push the controller and every time before presenting controller it is calculated again.

hansemannn · 2017-11-21T16:00:09Z

iphone/Classes/TiUIListView.m

-    viewController = [[TiApp app] controller];
+  BOOL shouldAnimate = YES;
+  if ([TiUtils isIOS9OrGreater]) {
+    shouldAnimate = NO;
  }


Why not BOOL shouldAnimate = ![TiUtils isIOS9OrGreater]; ?

Good idea! Done.

hansemannn · 2017-11-21T16:00:49Z

iphone/Classes/TiUIListView.m

@@ -2328,7 +2372,7 @@ - (void)initSearchController:(id)sender
    searchController = [[[UISearchController alloc] initWithSearchResultsController:resultViewController] retain];
    searchController.delegate = self;
    searchController.searchResultsUpdater = self;
-    searchController.hidesNavigationBarDuringPresentation = NO;
+    searchController.hidesNavigationBarDuringPresentation = YES;


Please try to prevent that breaking change, I am pretty sure people will complain later.

This is default behaviour. Though we can expose this property. Will continue to look in same.

hansemannn · 2017-11-21T16:01:32Z

iphone/Classes/TiUITableView.m

+#if IS_XCODE_9
+  if ([TiUtils isIOS11OrGreater]) {
+    topMargin += self.safeAreaInsets.top;
+    tableview.frame = CGRectMake(0, self.safeAreaInsets.top, tableview.frame.size.width, tableview.frame.size.height - self.safeAreaInsets.top);


Should that always be done? I thought only when having the extendSafeArea property set?

hansemannn · 2017-11-21T16:01:41Z

iphone/Classes/TiUITableView.m

+#endif
+  UIView *searchSuperView = [searchController.view superview];
+  if (!searchSuperView) {
+    return;


When can that happen?

No need of this as
if (![searchController isActive]) {
return;
}
is already checking for same. So removed.

hansemannn · 2017-11-21T16:02:09Z

iphone/Classes/TiUITableView.m

+  searchController.view.frame = CGRectMake(convertedOrigin.x, topMargin, self.frame.size.width, self.frame.size.height);
+  dimmingView.frame = CGRectMake(searchController.view.frame.origin.x, searchController.view.frame.origin.y, self.frame.size.width, self.frame.size.height);
+
+  float width = [searchField view].frame.size.width;


Use CoreGraphics types, e.g. CGFloat.

hansemannn · 2017-11-21T16:02:21Z

iphone/Classes/TiUITableView.m

+    resultSuperview.frame = CGRectMake(0, view.frame.origin.y + searchController.searchBar.frame.size.height, self.frame.size.width, self.frame.size.height);
+    resultViewController.tableView.frame = CGRectMake(0, 0, self.frame.size.width, self.frame.size.height);
+
+  } else {


Empty line, please lint everything.

hansemannn · 2017-11-21T16:02:46Z

iphone/Classes/TiUITableView.m

@@ -1809,7 +1857,7 @@ - (void)initSearhController
    resultViewController = [[UITableViewController alloc] init];
    resultViewController.tableView = [self searchTableView];
    searchController = [[[UISearchController alloc] initWithSearchResultsController:resultViewController] retain];
-    searchController.hidesNavigationBarDuringPresentation = NO;
+    searchController.hidesNavigationBarDuringPresentation = YES;


Same as above, try to prevent or we need to explicitly mark that in the docs.

hansemannn · 2017-11-21T16:03:22Z

iphone/Classes/TiUITableView.m

+#if IS_XCODE_9
+  if ([TiUtils isIOS11OrGreater]) {
+    tableview.frame = CGRectMake(0, 0, self.frame.size.width, self.frame.size.height);
+  }


I don't feel save with this. What if the table is not positioned full screen? And what about orientation changes? Please add tests for those scenarios.

Table's contentOffset should be managed rather table view frame. Otherwise view below to tableview will get visible. So managed contentOffset rather frame.

hansemannn · 2017-11-21T16:04:01Z

iphone/Classes/TiUITableView.m

+  BOOL shouldAnimate = YES;
+  if ([TiUtils isIOS9OrGreater]) {
+    shouldAnimate = NO;
+  }


Same as before, please refactor the assignment to be less if-based.

… view on top

mukherjee2

Passed FR with this environment:
Node Version: 8.9.1
NPM Version: 5.5.1
Mac OS: 10.13.1
Appc CLI: 7.0.0-master.48
Appc CLI NPM: 4.2.11-4
Titanium SDK version: 7.0.0 locally built from this PR
Appcelerator Studio vers 4.10.0
Xcode 9.1/iOS 11.1 iPhone 7 Plus

I tried both test cases that were in the ticket, and the view above the searcher did appear.

mukherjee2 · 2017-11-28T00:18:54Z

@vijaysingh-axway unit tests appear to be failing. Can you please look into those? I'll merge once tests pass.

vijaysingh-axway · 2017-11-28T08:24:36Z

@mukherjee2, Unit test failure issue fixed.
I hope you would have verified Test cases mentioned in tickets TIMOB-25528 and TIMOB-25532, which I have mentioned in comment. Thanks!

hansemannn · 2017-11-28T09:32:41Z

iphone/Classes/TiUITableView.m

@@ -2741,9 +2741,9 @@ - (void)presentSearchController:(UISearchController *)controller
      proxy = [proxy parent];
    }
    if ([proxy isKindOfClass:[TiWindowProxy class]]) {
-      searchControllerPresenter = [proxy windowHoldingController];
+      searchControllerPresenter = [[proxy windowHoldingController] retain];


So it did need to retain? :-) Please also run the static analyser to ensure there are no static leaks.

vijaysingh-axway added 7 commits November 17, 2017 13:36

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

31ec6d8

… view on top

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

71a1049

… view on top

[TIMOB-25491]: iOS Ti.UI.SearchBar not slide down when set visible a …

fa49f1b

…view on top

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

2f98449

… view on top

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

473ced3

… view on top

[TIMOB-25491] : Lint issue fixed

8db735b

Merge branch 'master' into TIMOB-25491

5a9492f

build added the ios label Nov 21, 2017

hansemannn requested changes Nov 21, 2017

View reviewed changes

vijaysingh-axway and others added 7 commits November 22, 2017 10:54

Merge branch 'master' into TIMOB-25491

e053c4b

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

19ccf20

… view on top

[TIMOB-25491] : Removed unnecessary code

31e504b

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a…

6fc1cad

… view on top

[TIMOB-25491] : Linting issue fixed

73860cf

Merge branch 'master' into TIMOB-25491

5b5001a

Merge branch 'master' into TIMOB-25491

587fb53

hansemannn approved these changes Nov 27, 2017

View reviewed changes

mukherjee2 self-requested a review November 28, 2017 00:14

mukherjee2 approved these changes Nov 28, 2017

View reviewed changes

build added the ios label Nov 28, 2017

[TIMOB-25491] : Unit test failure issue fixed

1906f9e

build added the ios label Nov 28, 2017

hansemannn reviewed Nov 28, 2017

View reviewed changes

Merge branch 'master' into TIMOB-25491

81db003

eric34 merged commit 239473a into tidev:master Nov 28, 2017

build added the ios label Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a view on top #9622

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a view on top #9622

vijaysingh-axway commented Nov 21, 2017

build commented Nov 21, 2017 •

edited

hansemannn left a comment

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

hansemannn Nov 21, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

hansemannn Nov 21, 2017

vijaysingh-axway Nov 22, 2017

mukherjee2 left a comment

mukherjee2 commented Nov 28, 2017

vijaysingh-axway commented Nov 28, 2017 •

edited

hansemannn Nov 28, 2017

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a view on top #9622

[TIMOB-25491] : iOS Ti.UI.SearchBar not slide down when set visible a view on top #9622

Conversation

vijaysingh-axway commented Nov 21, 2017

build commented Nov 21, 2017 • edited

hansemannn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukherjee2 left a comment

Choose a reason for hiding this comment

mukherjee2 commented Nov 28, 2017

vijaysingh-axway commented Nov 28, 2017 • edited

Choose a reason for hiding this comment

build commented Nov 21, 2017 •

edited

vijaysingh-axway commented Nov 28, 2017 •

edited