Skip to content

Empty view for media library view #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

mkchoi212
Copy link
Collaborator

Nothing much to it!
Just displays the empty view when the media controller is empty 😄
Please note that I made some adjustments to the UI as I saw fit to the new UI.

Copy link
Member

@carolanitz carolanitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really unsure of the if (self) block. I don't think that's how you would initialize this view

@@ -39,6 +40,20 @@

@implementation EmptyLibraryView

- (id)initWithFrame:(CGRect)frame isInFolder:(bool)inFolder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inFolder bool can go, we won't have folders anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I go ahead and delete all localized strings for FOLDER_EMPTY and FOLDER_EMPTY_LONG?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely

{
if (self) {
NSArray *subviewArray = [[NSBundle mainBundle] loadNibNamed:@"VLCEmptyLibraryView" owner:self options:nil];
self = [subviewArray objectAtIndex:0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should always use .firstObject because it will give you nil if there are not subviews.
Right now this would crash if something goes wrong with loading the Nib

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look correct to me. Shouldn't it be if (!self) and then you initialize? I need to take another look but it feels to me like this code doesn't belong here as well

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 5 cents:
Usually override of init require call
self = [super init];
In this case self will be setup properly and we need to check for "if (self)".
In case if we need load view from xib file the common solution is to make static method to do this. For example:

+ (instanceType)emptyLibraryViewInFolder:(bool)inFolder
{
    NSArray *subviewArray = [[NSBundle mainBundle] loadNibNamed:@"VLCEmptyLibraryView" owner:self options:nil];
    EmptyLibraryView *result = subviewArray.firstObject;
    [result setupEmptyValuesInFolder:inFolder];
    return result;
}

- (void)setupEmptyValuesInFolder:(bool)inFolder
{
    _emptyLibraryLabel.text = inFolder ? NSLocalizedString(@"FOLDER_EMPTY", nil) : NSLocalizedString(@"EMPTY_LIBRARY", nil);
    _emptyLibraryLongDescriptionLabel.text = inFolder ? NSLocalizedString(@"FOLDER_EMPTY_LONG", nil) : NSLocalizedString(@"EMPTY_LIBRARY_LONG", nil);
    [_learnMoreButton setTitle:NSLocalizedString(@"BUTTON_LEARN_MORE", nil) forState:UIControlStateNormal];
    _learnMoreButton.hidden = inFolder;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thumbs up for the mentioning of super

@@ -27,7 +28,13 @@ public class MediaDataSourceAndDelegate:NSObject, UICollectionViewDataSource, UI
}

public func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
return Int(services.mediaDataSource.numberOfFiles())
let numItems = Int(services.mediaDataSource.numberOfFiles())
if numItems == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we write
Bool hasMedia = numItems > 0
collectionView.backgroundView?.isHidden = hasMedia

@@ -53,6 +54,7 @@ public class VLCMediaViewController: UICollectionViewController, UISearchResults
setupCollectionView()
setupSearchController()
setupNavigationBar()
setUpEmptyBackgroundView()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setUp vs setup

I do not have any preference among them, but just because of karma I think they should be all the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't even notice that. Thanks @dcordero

return Int(services.mediaDataSource.numberOfFiles())
let numItems = Int(services.mediaDataSource.numberOfFiles())
let hasMedia = numItems > 0
collectionView.backgroundView?.isHidden = hasMedia
Copy link
Member

@carolanitz carolanitz Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code shouldn't go here. A view should not be modified from a model class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also consider not just hiding the emptyview but consider removing it. It might lead to side effects having this view still in the view hierarchy

@@ -9,6 +9,7 @@
#import "VLCConstants.h"
#import "VLCDownloadViewController.h"
#import "VLCLibrarySearchDisplayDataSource.h"
#import "VLCLibraryViewController.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the LibraryViewController?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VLCLibraryViewController.h is where EmptyLibraryView is declared and used within VLCLibraryViewController.h. Maybe writing a new version of it in Swift is beneficial in the long run?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think it should be taken out since VLCLibraryViewController will be removed "soon".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't have been declared in the LibraryViewController in the first place. It needs to live in it's own file. You don't have to write a new version in Swift, just move it into it's own class via copy paste :)

@@ -48,7 +48,7 @@ - (void)viewDidLoad
{
[super viewDidLoad];

self.nothingFoundLabel.text = NSLocalizedString(@"FOLDER_EMPTY", nil);
Copy link
Member

@carolanitz carolanitz Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't change any AppleTV related code in this PR

@@ -406,8 +406,8 @@ - (void)_displayEmptyLibraryViewIfNeeded

if ([_mediaDataSource numberOfFiles] == 0) {
_inFolder = (_libraryMode == VLCLibraryModeFolder || _libraryMode == VLCLibraryModeCreateFolder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the VLCLibrarycontroller doesn't need to be touched here. It's gonna be deleted anyway

@@ -37,6 +37,7 @@
@property (nonatomic, strong) IBOutlet UIActivityIndicatorView *activityIndicator;
@property (nonatomic, strong) IBOutlet UIButton *learnMoreButton;

- (id)initWithFrame:(CGRect)frame;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't need to be defined since EmptyView is a UIView subclass

{
self = [super initWithFrame:frame];
if (self) {
NSArray *subviewArray = [[NSBundle mainBundle] loadNibNamed:@"VLCEmptyLibraryView" owner:self options:nil];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initiWithFrame is actually the wrong place to load from the nib. You should take a look how it's done everywhere else. You're also calling super and assigning it to self, then checking for nil and then assigning self = subviewArray.firstObject; without having the nil check again. I missed that the first time this needs to be done differently

return Int(services.mediaDataSource.numberOfFiles())
let numItems = Int(services.mediaDataSource.numberOfFiles())
let hasMedia = numItems > 0
collectionView.backgroundView?.isHidden = hasMedia
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also consider not just hiding the emptyview but consider removing it. It might lead to side effects having this view still in the view hierarchy

@mkchoi212 mkchoi212 force-pushed the empty-view branch 6 times, most recently from 224352f to 0651009 Compare April 18, 2018 19:19
@@ -105,6 +109,19 @@ public class VLCMediaViewController: UICollectionViewController, UISearchResults
func setupNavigationBar() {
navigationItem.leftBarButtonItem = UIBarButtonItem(title: NSLocalizedString("SORT", comment: ""), style: .plain, target: self, action: #selector(sort))
}

func setupEmptyBackgroundView() {
emptyView = VLCEmptyLibraryView.init()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't have the init() here and should instantiate the View here from the nib

@@ -19,6 +20,7 @@ import Foundation

public class VLCMediaViewController: UICollectionViewController, UISearchResultsUpdating, UISearchControllerDelegate {
private var services: Services
private var emptyView: VLCEmptyLibraryView?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use a lazy var here and do the instantiation here

}

func displayEmptyViewIfNeeded() {
guard let view = emptyView, collectionView?.numberOfItems(inSection: 0) == 0 else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the emptyview will be lazy it's not optional anymore and you can simply write
collectionView?.backgroundView = collectionView?.numberOfItems(inSection: 0) == 0 ? emptyView : nil


@implementation VLCEmptyLibraryView

+ (instancetype)init
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be initWithCoder and you shouldn't have an instance of VLCEmptyView in it's own init, also no asserts but give the view in the xib an identifier and you load the nib with identifier

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that should remove the whole looking through subviews and you just got one instance but this code belongs in the mediaviewcontroller. Also New Classes should be in Swift 😀

NSAssert([[subviews firstObject] isKindOfClass:[VLCEmptyLibraryView class]], @"Loaded VLCEmptyLibraryView nib is invalid");

VLCEmptyLibraryView *view = subviews.firstObject;
view.emptyLibraryLabel.text = NSLocalizedString(@"EMPTY_LIBRARY", nil);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should move this code in override awakeFromNib()

lazy var emptyView: VLCEmptyLibraryView = {
let name = String(describing: VLCEmptyLibraryView.self)
let nib = Bundle.main.loadNibNamed(name, owner: self, options: nil)
guard let emptyView = nib?.first as? VLCEmptyLibraryView else { fatalError("Can't find nib for \(name)") }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to crash the app if we can't display an emptyview. An assertion and returning a UIView would've been just as fine

Note this uses pre-existing VLCEmptyLibraryView Objective-C code
Minor changes to color of background and sizes of labels
Specifically `FOLDER_EMPTY_*`
@carolanitz
Copy link
Member

merged with 6c1b909 & fbcf87b

@carolanitz carolanitz closed this Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants