Maintenance: Internal rework related to searchbar #1044

wutschel · 2024-04-14T15:02:52Z

Description

relates to #1042

Background:

The current implementation is not optimal and shows some unpredictable behaviour. Some of the current seen issues are caused by iOS17 (missing scroll indicator when adding a searchbar as UITableView's tableHeaderView, or too wide searchbars).
But there are general flaws in the implementation like re-creating the searchbar with each switch between list view and collection view, removing and adding searchbar from views multiple times. I prepared a change which reworks this but has a few workarounds which I would like to have 2nd opinion on.

Premises / Constraints:

Keep searchbar on top of lists, so pulldown-to-search
Keep active searchbar detached from list to be able to scroll the filtered result list
Keep searchbar inside the current library view to support iPad (searchbar sitting inside at the top of the stacked views)
Keep colored searchbar for album and season views
Active and inactive searchbar should look same (wherever possible)
UITableView's tableHeaderView must be a UISearchBar to handle insets gently

Changes:

Only create self.searchController once in viewDidLoad
self.searchController is the UISearchController which is used for an active search and always displayed detached (floating on top of list)
To activate a search a dedicated UISearchController + transparent overlayed UIView is created for list view and collection view each. The overlay has a UITapGestureRecognizer which triggers a method to show and activate the central self.searchController. This allows keep the same look of the searchbar.
Only create the inactive searchbar + overlay once for list view and once for collection view in viewDidLoad
Some refactoring to be able to unify the look of the inactive searchbar (the one with the overlay) and the active self.searchController

Summary for release notes

Maintenance: Internal rework related to searchbar

kambala-decapitator

will check current master no my side now, maybe will figure out what's up with the indicators
the PR is probably fine, but that "useless" inactive searchbar is a very strange workaround...
please resolve conflicts

XBMC Remote/DetailViewController.m

wutschel · 2024-05-10T12:41:41Z

will check current master no my side now, maybe will figure out what's up with the indicators

the PR is probably fine, but that "useless" inactive searchbar is a very strange workaround...

please resolve conflicts

Thanks for looking into this. I am lost and have no idea how to resolve this.

kambala-decapitator · 2024-05-10T13:52:49Z

what I see is that alpha of scroll indicators is set to 0 and looks like it's not possible to change that from user code - whatever I do, it's always reset to 0.

my suspicion is that view hierarchy is involved, specifically view controller hierarchy. If you open View Debugger, you can see that DetailViewController is not in the view tree - its subviews are directly under MasterViewController, which is abnormal. Although if you print view controller hierarchy in console, then DetailViewController is there. Probably ECSlidingViewController is involved, which is a quite old library as I see.

wutschel · 2024-05-10T19:32:10Z

ECSlidingViewController is not involved on iPad, where the scroll indicator is also missing. For iPad there is another (old) controller involved: StackScrollViewController.

I also now looked into the View Debugger and can see DetailViewController. It is found in InitialSlidingViewController > UIView > CustomNavigationController > UILayoutContainerView > UINavigationTransitionView > UIViewControllerWrapperView. https://ibb.co/wrCdr19

One more detail: When running in iOS 15.5 simulator both horizontal and vertical scroll indicators are present. When running in iOS 17 simulator only the horizontal one is there.
Both there: https://ibb.co/F5bffnM (upper instance = horizontal)
Vertical missing: https://ibb.co/kJBbwKL

kambala-decapitator · 2024-05-11T07:37:31Z

oh wow, seems I was looking at the wrong place :) will double-check

wutschel · 2024-05-11T11:30:48Z

oh wow, seems I was looking at the wrong place :) will double-check

For the issue itself it did not matter though. The same issue is also happening for the table which you were looking at (the main menu).

We should better move our discussion on the scroll indicators to #1007.

kambala-decapitator · 2024-05-11T12:44:45Z

At least it explains why my hacks Don didn't seem to have any effect

wutschel · 2024-05-11T13:06:32Z

Even when not directly adding MasterViewController in AppDelegate (not using InitialSildingViewController / ECSlidingViewController at all) the problem persists.

MasterViewController *masterViewController = [[MasterViewController alloc] initWithNibName:@"MasterViewController" bundle:nil];
masterViewController.mainMenu = mainMenuItems;
self.window.rootViewController = masterViewController;

Is there possibly something very basic wrong or not yet configured right related to window / scene?

kambala-decapitator · 2024-05-12T07:43:52Z

don't think it has any relation to window / scene

found a hacky workaround: just add this to viewDidAppear

    dispatch_async(dispatch_get_main_queue(), ^{
        dataList.showsVerticalScrollIndicator = YES;
    });

kambala-decapitator · 2024-05-12T07:48:17Z

XBMC Remote/DetailViewController.m

    }
 }

+- (void)openSearchBar {
+    showbar = YES;


please also rename this ivar to showBar

True, named it showSearchbar.

wutschel · 2024-05-12T07:51:23Z

don't think it has any relation to window / scene

found a hacky workaround: just add this to viewDidAppear
    dispatch_async(dispatch_get_main_queue(), ^{
        dataList.showsVerticalScrollIndicator = YES;
    });

Cool, works! :)
I will now review for which other places this needs to be done as well. I propose to add a Utilities method for this (parameter = UITableView instance) and always add a "Workaround" comment to each occurance.

kambala-decapitator · 2024-05-12T07:52:47Z

I propose to add a Utilities method for this (parameter = UITableView instance) and always add a "Workaround" comment to each occurance.

fully agree, very good idea

wutschel · 2024-05-12T07:55:07Z

I propose to add a Utilities method for this (parameter = UITableView instance) and always add a "Workaround" comment to each occurance.

fully agree, very good idea

Good. Will create a separate PR in next 30 mins. We can then either close the related issue with this, or keep it open for a real fix. You have any clue what is going wrong?

Only create self.searchController once in viewDidLoad. self.searchController is the UISearchController which is used for an active search and always displayed detached (floating on top of list). To activate a search a dedicated UISearchController + transparent overlayed UIView is created for list view and collection view each. The overlay has a UITapGestureRecognizer which triggers a method to show and activate the central self.searchController. This allows keep the same look of the searchbar. Only create the inactive searchbar + overlay once for list view and once for collection view in viewDidLoad. Some refactoring to be able to unify the look of the inactive searchbar (the one with the overlay) and the active self.searchController

wutschel · 2024-05-12T08:01:00Z

Squashed, proper commit message and rebased to master.

kambala-decapitator · 2024-05-12T08:07:23Z

We can then either close the related issue with this, or keep it open for a real fix. You have any clue what is going wrong?

absolutely no clue and don't really wish to spend any more time on that. Let's close the issue using the found workaround.

wutschel marked this pull request as ready for review April 21, 2024 09:17

wutschel force-pushed the rework_searchbar branch 2 times, most recently from bc5d193 to 639aea7 Compare April 21, 2024 12:08

kambala-decapitator reviewed May 10, 2024

View reviewed changes

XBMC Remote/DetailViewController.m Outdated Show resolved Hide resolved

wutschel force-pushed the rework_searchbar branch from 0a6eacb to c53d9c8 Compare May 10, 2024 12:42

wutschel force-pushed the rework_searchbar branch from c53d9c8 to 1691f38 Compare May 11, 2024 15:42

kambala-decapitator approved these changes May 12, 2024

View reviewed changes

wutschel added 3 commits May 12, 2024 09:58

Use member instead of method for showCancelButton

770f2dd

Rename ivar to showSearchbar

257130b

wutschel force-pushed the rework_searchbar branch from b34a487 to 257130b Compare May 12, 2024 08:00

kambala-decapitator merged commit 0b67576 into xbmc:master May 12, 2024

wutschel mentioned this pull request May 12, 2024

Bugfix: Make scroll indicators visible in iOS17 #1056

Merged

wutschel deleted the rework_searchbar branch May 12, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance: Internal rework related to searchbar #1044

Maintenance: Internal rework related to searchbar #1044

wutschel commented Apr 14, 2024 •

edited by kambala-decapitator

Loading

kambala-decapitator left a comment

wutschel commented May 10, 2024

kambala-decapitator commented May 10, 2024

wutschel commented May 10, 2024 •

edited

Loading

kambala-decapitator commented May 11, 2024

wutschel commented May 11, 2024 •

edited

Loading

kambala-decapitator commented May 11, 2024

wutschel commented May 11, 2024

kambala-decapitator commented May 12, 2024

kambala-decapitator May 12, 2024

wutschel May 12, 2024

wutschel commented May 12, 2024

kambala-decapitator commented May 12, 2024

wutschel commented May 12, 2024

wutschel commented May 12, 2024

kambala-decapitator commented May 12, 2024

Maintenance: Internal rework related to searchbar #1044

Maintenance: Internal rework related to searchbar #1044

Conversation

wutschel commented Apr 14, 2024 • edited by kambala-decapitator Loading

Description

Background:

Premises / Constraints:

Changes:

Summary for release notes

kambala-decapitator left a comment

Choose a reason for hiding this comment

wutschel commented May 10, 2024

kambala-decapitator commented May 10, 2024

wutschel commented May 10, 2024 • edited Loading

kambala-decapitator commented May 11, 2024

wutschel commented May 11, 2024 • edited Loading

kambala-decapitator commented May 11, 2024

wutschel commented May 11, 2024

kambala-decapitator commented May 12, 2024

kambala-decapitator May 12, 2024

Choose a reason for hiding this comment

wutschel May 12, 2024

Choose a reason for hiding this comment

wutschel commented May 12, 2024

kambala-decapitator commented May 12, 2024

wutschel commented May 12, 2024

wutschel commented May 12, 2024

kambala-decapitator commented May 12, 2024

wutschel commented Apr 14, 2024 •

edited by kambala-decapitator

Loading

wutschel commented May 10, 2024 •

edited

Loading

wutschel commented May 11, 2024 •

edited

Loading