[TIMOB-16622] iOS:Replace UISearchDisplayController with UISearchController #9066

vijaysingh-axway · 2017-05-17T09:20:15Z

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

…ller

hansemannn

Major changes required, please address those before going into the next review phase.

hansemannn · 2017-05-17T10:23:00Z

iphone/Classes/TiUIListView.m

@@ -39,9 +39,8 @@ @implementation TiUIListView {
    TiUIRefreshControlProxy* _refreshControlProxy;
 #endif

-    TiUISearchBarProxy *searchViewProxy;
-    UITableViewController *tableController;


This should not be removed.

In UISearchController there is option to pass nil for result controller. In this case it will use the current tableview for showing the search result. We can not use different controller to show the result because it will create similar problem which is mentioned in TIMOB-16622 and TIMOB-16826. So there is no need of tableViewController.
searchViewProxy is there.

hansemannn · 2017-05-17T10:23:14Z

iphone/Classes/TiUIListView.m

@@ -528,9 +526,6 @@ - (void)updateSearchResults:(id)unused
        [self buildResultsForSearchText];
    }
    [_tableView reloadData];
-    if ([searchController isActive]) {
-        [[searchController searchResultsTableView] reloadData];
-    } 


Why did this move away?

SearchViewController is using current tableView. And it do not have searchResultsTableView.

hansemannn · 2017-05-17T10:23:25Z

iphone/Classes/TiUIListView.m

            [_tableView reloadData];
-        }


Why did this move away?

hansemannn · 2017-05-17T10:23:55Z

iphone/Classes/TiUIListView.m

@@ -871,6 +861,7 @@ -(void)setSearchView_:(id)args
            [[searchViewProxy searchBar] setPlaceholder:@"Search"];
        }
        self.searchString = [[searchViewProxy searchBar] text];
+        [self initSearchController:self];


What happens if the search-view is set multiple times?

Not a problem . Every time searchViewController is released and new searchViewController is created.

hansemannn · 2017-05-17T10:24:27Z

iphone/Classes/TiUIListView.m

@@ -2004,7 +1993,7 @@ - (void)searchBarTextDidBeginEditing:(UISearchBar *)searchBar
    }
    self.searchString = (searchBar.text == nil) ? @"" : searchBar.text;
    [self buildResultsForSearchText];
-    [[searchController searchResultsTableView] reloadData];
+    [_tableView reloadData];


_tableView is the main table view, but the deleted line reloaded the search-results table view.

In this case both tableView is same.

hansemannn · 2017-05-17T10:27:46Z

iphone/Classes/TiUIListView.m

-        searchController.searchResultsDataSource = self;
-        searchController.searchResultsDelegate = self;
+    if (sender == self && searchController == nil)
+    {


We put brackets in the same line

hansemannn · 2017-05-17T10:28:36Z

iphone/Classes/TiUIListView.m

-        searchController.searchResultsDelegate = self;
+    if (sender == self && searchController == nil)
+    {
+        searchController = [[UISearchController alloc] initWithSearchResultsController:nil];


Why are you initializing it with nil? I guess we should rather manage an own table-view for it, maybe even in an own helper-class.

In UISearchController there is option to pass nil for result controller. In this case it will use the current tableview for showing the search result. We can not use different controller to show the result because it will create similar problem which is mentioned in TIMOB-16622 and TIMOB-16826.

hansemannn · 2017-05-17T10:29:04Z

iphone/Classes/TiUIListView.m

        searchController.delegate = self;
+        searchController.searchResultsUpdater = self;
+        searchController.hidesNavigationBarDuringPresentation = NO;
+        searchController.searchBar.frame = CGRectMake(searchController.searchBar.frame.origin.x, searchController.searchBar.frame.origin.y, searchController.searchBar.frame.size.width, 44.0);


I'm sceptical about giving it a fixed size. Have you tried this on landscape / multiple orientation changes?

I tried but don't found. If you face please let me know.

hansemannn · 2017-05-17T10:29:24Z

iphone/Classes/TiUIListView.m

        searchController.delegate = self;
+        searchController.searchResultsUpdater = self;
+        searchController.hidesNavigationBarDuringPresentation = NO;


We should stick to the default values, is setting this value necessary?

Default value will hide navigation bar and is misaligning the tableview.

hansemannn · 2017-05-17T10:32:16Z

iphone/Classes/TiUISearchBar.m

+    searchView = searchBar;
+    [searchView setDelegate:self];
+    [self addSubview:searchView];
+}


Not sure what's this used for. It's used in https://github.com/appcelerator/titanium_mobile/pull/9066/files#diff-c25f31a8654c7459f7b97be688ab44efR2210 but using a setter seems to be the wrong way. I'm curious if we should o that way, especially because it will add a new sub-view every time it's called. So if the user changes the search-bar over time (using searchView / searchBar, multiple search-bars will just be added above each other.

Also brackets miss-formatted again, that needs to be checked in general.

First it is removing the search bar then adding it as a subview. So not a problem.

hansemannn · 2017-05-18T04:10:16Z

apidoc/Titanium/UI/ListView.yml

-    constants: Titanium.UI.iPhone.ListViewSeparatorStyle.*
-    since: 6.1.0
-    platforms: [iphone, ipad]
-    availability: creation


Removing them is not an option. Can't we just grab the values and style the search-tableview accordingly?

Is it really necessary? Problem mentioned in ticket will not arise.

Agree, but we need to state that, by using:

removed: since: "6.2.0" notes: | The styling of the search table-view is determined by it's table-view, so there is no additional styling required.

hansemannn · 2017-05-18T04:10:56Z

iphone/Classes/TiUIListView.m

    [_tableView reloadData];
 }

 - (void)presentSearchController:(UISearchController *)controller
 {
-    id dimBgValue = [[self  proxy] valueForKey:@"dimsBackground"];
-    searchController.dimsBackgroundDuringPresentation = [TiUtils boolValue:dimBgValue def:NO];


Rather document it and set the value in an appropriate place (like init or here if that's more flexible).

This property is of no use, until searchResultController is passed in UISearchViewController. So removed it and put a default value while initializing searchViewController.

…th UISearchController

…anium_mobile into TIMOB-16622

hansemannn · 2017-05-29T06:59:04Z

apidoc/Titanium/UI/ListView.yml

+    since: "6.2.0"
+    notes: |
+        The background color of the search table-view is determined by it's table-view, so there is no additional property required.
+


This is not the way the docs are written. Please check how other docs handle removal, also remember to break long lines into separate ones.

hansemannn

There is still one (commented in) reference to the UISearchDisplayController in the searchDisplayControllerDidEndSearch: delegate, please remove that one as well, although it will not throw a warning anyway.

hansemannn · 2017-06-01T12:12:01Z

In my FT right now. I noticed that the old behavior dimmed the background by default, now it doesn't (setting dimsBackgroundDuringPresentation to NO, which is against the default behavior). Was there a special reason you did it that way? Maybe I am overseeing something, thanks!

EDIT: Happening for both listview and tableview. All other tests are looking good!

vijaysingh-axway · 2017-06-02T03:34:27Z

In our current implementation of UISearchController, we are passing nil for result controller (reason explained in my first comment above). So it is using current table view for showing result. If we use default behaviour of property dimsBackgroundDuringPresentation, we can not be able to select any row from result tableView, as this will be disabled. So dimsBackgroundDuringPresentation is set to NO.

hansemannn · 2017-06-02T08:27:55Z

Well, I tried it with dimsBackgroundDuringPresentation and was able to select rows. However, show the edit actions did work with both cases.

vijaysingh-axway · 2017-06-02T09:05:07Z

Are you saying by setting " dimsBackgroundDuringPresentation = YES" inside TIUIlistView.m, you are able to select row while searching? This property is not exposed to be use with js.

hansemannn · 2017-06-02T10:12:59Z

Yep, inside the Obj-C code :-)

…hDisplayController in TIUIListView

…ntroller in TIUITableView

vijaysingh-axway · 2017-06-20T07:53:06Z

@hansemannn As we discussed, I have implemented this using new UITableViewController and a table view in order to show dim background while searching.

vijaysingh-axway · 2017-06-30T05:55:29Z

@hansemannn Ping!

vijaysingh-axway · 2017-07-19T02:47:32Z

@hansemannn Can you please review a minor fix in this PR , which I found while working for edit action for search result. Thanks!

hansemannn · 2017-07-19T03:08:19Z

iphone/Classes/TiUIListView.m

-        [_tableView reloadData];
+    [_tableView reloadData];
+    if ([searchController isActive]) {
+        [resultViewController.tableView reloadData];


Indentation. Also, when would searchActive be NO but [searchController isActive] be YES?

there are 2 ways we can use search in list view -

add searcbar on window, and show search result in same table view (see last test case in ticket)

make search bar part of list view (see test case in ticket except last)
In second way, when we click on searchbar [searchController isActive] will be yes.
But for searchActive being to Yes, there should be atleast one character in search bar.
So when second way of search used, user clicked on search bar and not typed any character, [searchController isActive] will be YES and searchActive will be NO.
If you see the old implementation, it is in same way.

Are you sure there is problem in indentation, I don't think so.

hansemannn · 2017-07-19T03:08:33Z

iphone/Classes/TiUIListView.m

+            [[resultViewController tableView] reloadData];
+        } else {
+            [_tableView reloadData];
+        }


Same question as above.

ewieberappc

FR Passed, using:
MacOS 10.12.6 (16G24b)
Studio 4.9.0.201705302345
Ti SDK 7.0.0.v20170802103048
Appc NPM 4.2.9
Appc CLI 6.2.3
Alloy 1.9.13
Xcode 8.3.3 (8E3004b)

Tested searchbar functionality when used with listview and tableview. No issues encountered. Tested using the provided sample code as well as the searchbar suite

vijaysingh-axway added 2 commits May 17, 2017 15:08

[TIMOB-16622]: Replaced UISearchDisplayController with UISearchContro…

5d2ee66

…ller

[TIMOB-16622]:Replace UISearchDisplayController with UISearchController

65e8ec4

hansemannn changed the title ~~[TIMOB-16622]:Replace UISearchDisplayController to UISearchController~~ [TIMOB-16622] iOS:Replace UISearchDisplayController with UISearchController May 17, 2017

hansemannn self-requested a review May 17, 2017 10:18

hansemannn added this to the 6.2.0 milestone May 17, 2017

hansemannn requested changes May 17, 2017

View reviewed changes

vijaysingh-axway added 2 commits May 18, 2017 11:51

[TIMOB-16622]: check applied when search is active

8f0bda9

[TIMOB-16622]:default of dimsbackgroundcolor set

326854a

hansemannn requested changes May 18, 2017

View reviewed changes

hansemannn and others added 9 commits May 18, 2017 15:53

Merge branch 'master' into TIMOB-16622

cc5626d

[TIMOB-16622]: In TiUITableView UISearchDisplayController replaced wi…

92c5bd0

…th UISearchController

Merge branch 'TIMOB-16622' of https://github.com/vijaysingh-axway/tit…

715fd6a

…anium_mobile into TIMOB-16622

code alignment

82777b6

[TIMOB-16622]:width of search bar fixed while rotation

da16438

Merge branch 'TIMOB-16622' of https://github.com/vijaysingh-axway/tit…

11151f1

…anium_mobile into TIMOB-16622

Merge branch 'master' into TIMOB-16622

9dcf45d

Merge branch 'master' into TIMOB-16622

8276351

[TIMOB-16622]:Doc updated

32ad6f8

hansemannn requested changes May 29, 2017

View reviewed changes

[TIMOB-16622]:Doc updated with proper formatting

7c492f6

hansemannn requested changes Jun 1, 2017

View reviewed changes

vijaysingh-axway added 3 commits June 19, 2017 17:10

[TIMOB-16622]: UISearchController used in place of deprecated UISearc…

0c21389

…hDisplayController in TIUIListView

[TIMOB-16622]:UISearhViewController used instead of UISearchDisplayCo…

019f1f5

…ntroller in TIUITableView

[TIMOB-16622]: dimBackgroundForSearch property documented

0870b11

hansemannn and others added 2 commits July 3, 2017 18:00

Merge branch 'master' into TIMOB-16622

9089e31

[TIMOB-16622] Fix minor outstanding review-issues

7b84063

hansemannn mentioned this pull request Jul 4, 2017

[TIMOB-16622] Fix minor outstanding review-issues vijaysingh-axway/titanium_mobile#1

Merged

hansemannn approved these changes Jul 4, 2017

View reviewed changes

hansemannn added the in-qe-testing 🕵 label Jul 4, 2017

hansemannn added improvement ios labels Jul 18, 2017

[TIMOB-16622]: If search bar separately to listview

ad81b10

hansemannn reviewed Jul 19, 2017

View reviewed changes

mukherjee2 self-requested a review August 2, 2017 16:47

Merge branch 'master' into TIMOB-16622

333ac7d

hansemannn mentioned this pull request Aug 8, 2017

[TIMOB-24351]:iOS Support "editActions" in listview search-results #9227

Merged

ewieberappc requested review from ewieberappc and removed request for mukherjee2 August 8, 2017 21:07

Merge branch 'master' into TIMOB-16622

d3eacd2

ewieberappc approved these changes Aug 8, 2017

View reviewed changes

hansemannn modified the milestones: 7.0.0, 6.2.0 Aug 8, 2017

Merge branch 'master' into TIMOB-16622

4396c00

ewieberappc merged commit ce8b417 into tidev:master Aug 8, 2017

[TIMOB-16622] iOS:Replace UISearchDisplayController with UISearchController #9066

[TIMOB-16622] iOS:Replace UISearchDisplayController with UISearchController #9066

Conversation

vijaysingh-axway commented May 17, 2017

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

vijaysingh-axway May 18, 2017 • edited

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

Choose a reason for hiding this comment

hansemannn left a comment

Choose a reason for hiding this comment

hansemannn commented Jun 1, 2017 • edited

vijaysingh-axway commented Jun 2, 2017

hansemannn commented Jun 2, 2017

vijaysingh-axway commented Jun 2, 2017

hansemannn commented Jun 2, 2017

vijaysingh-axway commented Jun 20, 2017

vijaysingh-axway commented Jun 30, 2017

vijaysingh-axway commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewieberappc left a comment

Choose a reason for hiding this comment

vijaysingh-axway May 18, 2017 •

edited

hansemannn commented Jun 1, 2017 •

edited