UI Improvements to the Main Collection View #405

yoshisuga · 2022-06-03T06:00:54Z

Updated the collection view so that:

The Recently Played section now scrolls horizontally and supports up to 100 entries.
Updated the expanding/collapsing UI so that it can be expanded/collapsed using a +/- button on the right side, instead of a chevron in the header. You can still tap the header to expand/collapse as well.
Added prefetching support: images are now cached using prefetching.
Changed the font of the header to Bold instead of Heavy

…tching to start fetching images earlier

… Support collapsing/expanding using a button instead of a chevron in the header

…. Adjusted position of button to accomodate for safe area

yoshisuga · 2022-06-03T06:03:55Z

xcode/MAME4iOS/ChooseGameController.m

    if (_layoutMode == LayoutList || _layoutCollums == 0)
        return layout.itemSize;

    CGPoint row_height = [self heightForRowAtIndexPath:indexPath];
    return CGSizeMake(layout.itemSize.width, row_height.x + row_height.y);
 }

+-(UICollectionViewFlowLayout*)layoutForRecentlyPlayedCell {


Not really that happy with this, but this is done to provide a different collection view layout when the layoutMode is LayoutList, and the Recently Played section is displayed.

The Recently Played section adopts the size from the main collection view layout, but not in the LayoutList mode. For the LayoutList mode, I'm just choosing the CELL_SMALL_WIDTH size.

Did having a horz scrolling "list cell" look weird?

Maybe the cells in the Recents view should be a little narrower, like 80% so the user can that you can scroll to the right

yoshisuga · 2022-06-03T06:06:05Z

xcode/MAME4iOS/ChooseGameController.m

-{
-    UICollectionViewFlowLayout* layout = (UICollectionViewFlowLayout*)self.collectionView.collectionViewLayout;
-    GameInfo* game = [self getGameInfo:indexPath];
+// compute the size(s) of a single game. returns: (x = image_height, y = text_height)


Using a CGPoint as a tuple of floats is clever, but this should really be a struct 🙂I left it there for now.

It can't be a struct because we put it in an NSDictionary.

It can be a struct in Swift! But not ObjC

ToddLa

First pass comments...

Overall pretty good, I would like to remove the ListView special case. It would make the code less complex and consistent. And I don't think ListView is used much, so if Recents looks. Little strange so be it.

xcode/MAME4iOS/ChooseGameController+Prefetching.swift

ToddLa · 2022-06-03T16:00:06Z

xcode/MAME4iOS/ChooseGameController+Prefetching.swift

+                  let imageUrl = game.gameImageURLs.first else {
+                continue
+            }
+            guard ImageCache.sharedInstance().getImage(imageUrl) == nil else {


We should add a NS_SWIFT_NAME so it is just ImageCache.shared

ToddLa · 2022-06-03T16:04:57Z

xcode/MAME4iOS/ChooseGameController.m

-{
-    UICollectionViewFlowLayout* layout = (UICollectionViewFlowLayout*)self.collectionView.collectionViewLayout;
-    GameInfo* game = [self getGameInfo:indexPath];
+// compute the size(s) of a single game. returns: (x = image_height, y = text_height)


It can't be a struct because we put it in an NSDictionary.

It can be a struct in Swift! But not ObjC

ToddLa · 2022-06-03T16:06:41Z

xcode/MAME4iOS/ChooseGameController.m

-    GameInfo* game = [self getGameInfo:indexPath];
+// compute the size(s) of a single game. returns: (x = image_height, y = text_height)
+-(CGPoint)heightForGameInfo:(GameInfo*)game usingLayout:(nullable UICollectionViewLayout*)layout {
+    UICollectionViewFlowLayout *flowLayout = (UICollectionViewFlowLayout*)self.collectionView.collectionViewLayout;


Hmm don't understand this, but I bet I will later. Prob related to ListView

ToddLa · 2022-06-03T16:08:36Z

xcode/MAME4iOS/ChooseGameController.m

    if (_layoutMode == LayoutList || _layoutCollums == 0)
        return layout.itemSize;

    CGPoint row_height = [self heightForRowAtIndexPath:indexPath];
    return CGSizeMake(layout.itemSize.width, row_height.x + row_height.y);
 }

+-(UICollectionViewFlowLayout*)layoutForRecentlyPlayedCell {


Did having a horz scrolling "list cell" look weird?

ToddLa · 2022-06-03T16:10:03Z

xcode/MAME4iOS/ChooseGameController.m

+
+// get size of an item
+- (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewFlowLayout *)layout sizeForItemAtIndexPath:(NSIndexPath *)indexPath {
+    if ([self hasRecentlyPlayed] && indexPath.section == 0) {


Other code checks for recents by doing a string compare of sectionTitles, this code assumes Recents is always first

Also shouldn't this info be cached like every other row?

ToddLa · 2022-06-03T16:13:06Z

xcode/MAME4iOS/ChooseGameController.m

    [cell setTextInsets:UIEdgeInsetsMake(CELL_INSET_Y, CELL_INSET_X, CELL_INSET_Y, CELL_INSET_X)];

    CGFloat image_height = 0.0;
-    if (_layoutMode != LayoutList && _layoutCollums > 1) {
+    if (isRecentlyPlayedSection) {
+        // For the recently played section, don't use the layout-based cached image heights


Can't the special case of the Recents section be in heightForRow atIndexPath, then it will get advantage of being coached too

ToddLa · 2022-06-03T16:18:55Z

xcode/MAME4iOS/GameInfoCell.swift

@@ -137,7 +137,7 @@ class GameInfoCell : UICollectionViewCell {
    override func layoutSubviews() {
        super.layoutSubviews()

-        var rect = bounds
+        var rect = bounds.inset(by: UIEdgeInsets(top: 0, left: 0, bottom: 0, right: 32))


I don't like the hard coded constants of 30 and 28 should be based on the height of the cell, or line height of the font

ALao what happens on tvOS, do we still have these buttons?

ToddLa · 2022-06-03T16:21:55Z

xcode/MAME4iOS/RecentlyPlayedCell.swift

+        backgroundColor = .clear
+        contentView.addSubview(collectionView)
+        collectionView.translatesAutoresizingMaskIntoConstraints = false
+        NSLayoutConstraint.activate([


Is This is really better than a A single line

collectionView.frame = contentView.bounds

ToddLa · 2022-06-03T16:24:29Z

xcode/MAME4iOS/RecentlyPlayedCell.swift

+
+import UIKit
+
+@objcMembers class RecentlyPlayedCell: UICollectionViewCell {


I have a lot of experience with nested UICollectionView's and I am surprised how well they work, this looks good.

I like how you did not duplicate all the cell setup, and just made this a container for "the same cells" from the parent.

ToddLa · 2022-06-03T16:31:59Z

xcode/MAME4iOS/ChooseGameController.m

@@ -811,6 +813,10 @@ - (void)filterGameList
    }
 }

+-(BOOL)hasRecentlyPlayed {


This should probably be changed to isRecentsSection:IndexPath

This way the assumption of first section (or title name) a
Can live here

ToddLa · 2022-06-03T17:04:29Z

xcode/MAME4iOS/GameInfoCell.swift

@@ -243,11 +243,45 @@ class GameInfoCell : UICollectionViewCell {
 // MARK: GameInfoHeader - same as GameInfoCell

 class GameInfoHeader : GameInfoCell {


I was expecting this header cell to get used like this!

ToddLa · 2022-06-03T17:05:16Z

xcode/MAME4iOS/ChooseGameController.m

@@ -330,11 +330,13 @@ - (void)viewDidLoad
    // collection view
    [self.collectionView registerClass:[GameInfoCell class] forCellWithReuseIdentifier:CELL_IDENTIFIER];


We should add identifiers to GameInfoCell and GameInfoHeader too

ToddLa · 2022-06-03T17:27:05Z

Oh the nested collection view is gonna need to add some code to handle game controller navigation, aka HandleButtonPress, PV has some nasty nested CVs and the code was gross, M4i it should just be a few lines.

I can look into this if you want @yoshisuga

Should be as easy as adding a handleButtonPress to the RecentsCell, and have the parent CV check if the cell supports habdleButtonPress and pass the button code to it, but only in the left/right/select case.

TVAlertConyrolllrt does a similar thing

ToddLa · 2022-06-03T17:40:50Z

xcode/MAME4iOS/ChooseGameController+Prefetching.swift

+    public func collectionView(_ collectionView: UICollectionView, prefetchItemsAt indexPaths: [IndexPath]) {
+        print("ChooseGameController: Start prefetch of images for indexPaths: \(indexPaths)")
+        for indexPath in indexPaths {            
+            guard let game = getGameInfo(indexPath),


I think you can just call the gameLoadImage (same thing when setting up cell) and ignore the result

ToddLa · 2022-06-04T14:27:18Z

xcode/MAME4iOS/ChooseGameController+Prefetching.swift

+//  Copyright © 2022 MAME4iOS Team. All rights reserved.
+//
+
+extension ChooseGameController: UICollectionViewDataSourcePrefetching {


I am worried that iOS is gonna go crazy pre-loading and we will fire off a lot of net requests, we can't cancel

yoshisuga added 3 commits June 2, 2022 05:29

Support for horizontally scrolling Recently Played section; Use prefe…

6bd740e

…tching to start fetching images earlier

Fix the recent played collection view when using the LayoutList mode;…

4304e40

… Support collapsing/expanding using a button instead of a chevron in the header

Adjusted frame of text label to accomodate for expand/collapse button…

26595b2

…. Adjusted position of button to accomodate for safe area

yoshisuga requested a review from ToddLa June 3, 2022 06:00

yoshisuga commented Jun 3, 2022

View reviewed changes

yoshisuga added 2 commits June 2, 2022 20:08

replace cell identifier with reference to static variable

4fa2ddf

updated WHATSNEW

dd748b6

ToddLa reviewed Jun 3, 2022

View reviewed changes

ToddLa reviewed Jun 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI Improvements to the Main Collection View #405

UI Improvements to the Main Collection View #405

yoshisuga commented Jun 3, 2022 •

edited

Loading

yoshisuga Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

yoshisuga Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa left a comment

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa Jun 3, 2022

ToddLa commented Jun 3, 2022 •

edited

Loading

ToddLa Jun 3, 2022

ToddLa Jun 4, 2022


		import UIKit

		@objcMembers class RecentlyPlayedCell: UICollectionViewCell {

		@@ -243,11 +243,45 @@ class GameInfoCell : UICollectionViewCell {
		// MARK: GameInfoHeader - same as GameInfoCell

		class GameInfoHeader : GameInfoCell {

		@@ -330,11 +330,13 @@ - (void)viewDidLoad
		// collection view
		[self.collectionView registerClass:[GameInfoCell class] forCellWithReuseIdentifier:CELL_IDENTIFIER];

UI Improvements to the Main Collection View #405

Are you sure you want to change the base?

UI Improvements to the Main Collection View #405

Conversation

yoshisuga commented Jun 3, 2022 • edited Loading

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

ToddLa 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

ToddLa commented Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshisuga commented Jun 3, 2022 •

edited

Loading

ToddLa commented Jun 3, 2022 •

edited

Loading