Navigation Menu

Skip to content
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

Settings: Actionsheet #102

Closed
wants to merge 4 commits into from
Closed

Conversation

mkchoi212
Copy link
Collaborator

@mkchoi212 mkchoi212 commented Jul 23, 2018

Checklist

  • I've run bundle exec fastlane test from the root directory to see all new and existing tests pass
  • I've followed the vlc-ios code style and run bundle exec fastlane lint to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Description

Must be rebased after the settings PR is merged

Instead of pushing a new view to adjust the settings, VLCActionSheet is used.

@mkchoi212 mkchoi212 changed the title Settings: Modal view Settings: Actionsheet Jul 23, 2018
@mkchoi212 mkchoi212 force-pushed the settings-action branch 2 times, most recently from 9f55883 to a0b7312 Compare July 23, 2018 07:19
let checkmark: UILabel = {
let checkmark = UILabel()
checkmark.text = "✓"
checkmark.font = UIFont.systemFont(ofSize: 15)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be bigger?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, It's a bit small but I wonder if this shouldn't be a separate class. For example perhaps a VLCSettingSheetCell?

Copy link
Member

Choose a reason for hiding this comment

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

Also, open thought, I'm aware that on Figma the checkmark is on the right but shouldn't we consider putting the check mark on the left like the edit mode? Might just be a personal preference but I think it will be nicer 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do other actionsheet cells need checkmarks? If not, it would definitely make sense to create a new class. Also, if other actionsheet cells need both images and checkmarks, I think it would be better to keep the checkmark on the right for consistency.
| image | text | checkmark | or
| null | text | checkmark |

Copy link
Member

Choose a reason for hiding this comment

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

Not all ActionSheet cells need/will need checkmarks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So for the few that need both checkmarks and images, it should be placed on the right, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just subclassed it to VLCSettingSheetCell and it makes more sense :D

@mkchoi212 mkchoi212 force-pushed the settings-action branch 3 times, most recently from e7a85d0 to b42bf75 Compare July 25, 2018 03:16
@mkchoi212 mkchoi212 changed the title Settings: Actionsheet [WIP] Settings: Actionsheet Jul 25, 2018
@mkchoi212 mkchoi212 changed the title [WIP] Settings: Actionsheet Settings: Actionsheet Jul 25, 2018
@@ -11,7 +11,9 @@

class VLCActionSheetCell: UICollectionViewCell {

static let identifier = "VLCActionSheetCell"
@objc static var identifier: String {
return String(describing: self)
Copy link
Member

Choose a reason for hiding this comment

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

That's interesting! Today I learned :) We should do that everywhere

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.

Some comments are small some are bigger :) Overall the code looks very clean 👍 Did you or @Mikanbu test these changes with the chromecast actionsheet?
Also I noticed that the title header is supposed to be in bold font ^^


specifierManager = [[VLCSettingsSpecifierManager alloc] initWithSpecifier:specifier settingsReader:self.settingsReader settingsStore:self.settingsStore];
actionSheet.delegate = specifierManager;
actionSheet.dataSource = specifierManager;
Copy link
Member

Choose a reason for hiding this comment

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

It should be enough to create the Actionsheet and specifierManager once. Right now you do it on every tab of the cell.

actionSheet.delegate = specifierManager;
actionSheet.dataSource = specifierManager;

[actionSheet setActionWithClosure:^(id item) {
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 use something else than id here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so because the closure is receiving bunch of different types as item. But even if we were able to type check item as something like long, InAppSettings doesn't provide a set___ method for all types

NSDictionary *info = [NSDictionary dictionaryWithObject:item forKey:[specifier key]];
[[NSNotificationCenter defaultCenter] postNotificationName:kIASKAppSettingChanged
object:[specifier key]
userInfo:info];
Copy link
Member

Choose a reason for hiding this comment

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

do you need to fire the notification here? Is it not done in the setter of the settingsstore?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 31, 2018

Choose a reason for hiding this comment

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

Wow, I didn't know that :D
It works without the 2 lines.

default:
assertionFailure("\(reuseIdentifier) has not been defined for VLCSettingsTableViewCell")
accessoryType = .none
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 handle it explicitly for the known cases and keep the assertionFailure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! Don't know what I was thinking :p

}

var selectedIndex: IndexPath {
var index: Int
Copy link
Member

Choose a reason for hiding this comment

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

you can make that a let since it will definitely be set in the if else part

return
}

updateCollectionViewCellApparence(cell: oldCell, highlighted: false)
Copy link
Member

Choose a reason for hiding this comment

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

instead of highlighted does the cell maybe know that it's highlighted or selected itself? Maybe you can overwrite the setter if that's the case

}

let titles = specifier.multipleTitles()
cell.name.text = settingsReader.title(forStringId: titles?[indexPath.row] as? String)
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 add a check for indexPath.row < titles.count? Otherwise we crash here and maybe as well that the object is of type String otherwise we fail here silently

* VLCSettingSheetCell.swift
*
* Copyright © 2018 VLC authors and VideoLAN
* Copyright © 2018 Videolabs
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 remove the Videolabs copyright ;)


[actionSheet setActionWithClosure:^(id item) {
[self.settingsStore setObject:item forKey:specifier.key];
[self.settingsStore synchronize];
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 the self here lead to a retain cycle?

@objc init(specifier: IASKSpecifier, settingsReader: IASKSettingsReader, settingsStore: IASKSettingsStore) {
self.specifier = specifier
self.settingsReader = settingsReader
self.settingsStore = settingsStore
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't there be a super.init() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch :D

@@ -15,7 +15,7 @@ class VLCActionSheetSectionHeader: UIView {

let title: UILabel = {
let title = UILabel()
title.font = UIFont.systemFont(ofSize: 13)
title.font = UIFont.boldSystemFont(ofSize: 13)
Copy link
Member

Choose a reason for hiding this comment

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

Is this change according to Figma?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep!

@@ -25,6 +27,7 @@ class VLCActionSheetCell: UICollectionViewCell {
name.textColor = PresentationTheme.current.colors.cellTextColor
name.font = UIFont.systemFont(ofSize: 15)
name.translatesAutoresizingMaskIntoConstraints = false
name.setContentHuggingPriority(.defaultLow, for: .horizontal)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set ContentHuggingPriority ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to extend the name label all the way to the right so the checkmark can be at the rightmost edge. It can be moved to the subclass if it's breaking things.

@@ -47,7 +50,7 @@ class VLCActionSheetCell: UICollectionViewCell {
setupViews()
}

private func setupViews() {
func setupViews() {
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 this method should be internal.

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 31, 2018

Choose a reason for hiding this comment

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

can I ask you why?? It's currently being overriden by VLCSettingsActionSheetCell

Copy link
Member

Choose a reason for hiding this comment

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

I believe that it would be quite wrong to let other classes have the possibility to initialize the properties of VLCActionSheetCell.

I'm not saying that VLCActionSheetCell is perfect but I think classes should try to stay "self-contained".

For example, I think this would be nicer:

class TestCell: VLCActionSheetCell {
    override init(frame: CGRect) {
        super.init(frame: frame) // super.setupViews() is called in the super.init()
        setupViews()
    }

    required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    private func setupViews() {
        stackView.removeArrangedSubview(icon)
        stackView.addArrangedSubview(//something\\)
    }
}

Also, I'm sorry if I don't make any sense 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha! And it totally makes sense :D

@objc init(settingsReader: IASKSettingsReader, settingsStore: IASKSettingsStore) {
super.init()
self.settingsReader = settingsReader
self.settingsStore = settingsStore
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the properties be initialized before calling super.init()? Therefore not needing to implicitly unwrap the properties on top?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah good catch. completely missed this :D

@mkchoi212
Copy link
Collaborator Author

mkchoi212 commented Aug 1, 2018

Just tested it with Chromecast and nothing seems to be broken @carolanitz

Btw, I deleted the "if only 1 renderer, automatically select it" to get this screenshot

simulator screen shot - iphone x - 2018-08-01 at 10 57 43

@mkchoi212
Copy link
Collaborator Author

🔔 ping!


func actionSheet(collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: VLCSettingsSheetCell.identifier, for: indexPath) as? VLCSettingsSheetCell else {
fatalError("VLCSettingsSpecifierManager: VLCActionSheetDataSource: Unable to dequeue reusable cell")
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 dequeueReusableCell return nil when there is no cell to reuse ? and shouldn't in that case a new Cell be created instead of having a fatalError ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes :D

@carolanitz
Copy link
Member

@mkchoi212 looks fine to me just that one question regarding dequeueReusableCell :)

@mkchoi212
Copy link
Collaborator Author

🔔 ping :D

@carolanitz
Copy link
Member

sorry this time I saw something else instead of having fatalerrors which litterally crash the app we should probably return rather empty arrays and just have assertionfailures. The only time you should use fatalerror is when you corrupt data or get into an unrecoverable appstate where only a restart would be the solution. Other options are errormessages to the user


func actionSheet(collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: VLCSettingsSheetCell.identifier, for: indexPath) as? VLCSettingsSheetCell else {
return UICollectionViewCell()
Copy link
Member

Choose a reason for hiding this comment

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

might make sense to still have an assertionfailure here because this should only happen when someone forgot to register the cellidentifier and the cellclass right ?

@carolanitz
Copy link
Member

merged with 0bcf87a and following

@carolanitz carolanitz closed this Aug 20, 2018
@mkchoi212 mkchoi212 deleted the settings-action branch August 20, 2018 20:02
@mkchoi212 mkchoi212 restored the settings-action branch August 20, 2018 20:03
@mkchoi212 mkchoi212 deleted the settings-action branch August 20, 2018 20:03
@mkchoi212 mkchoi212 restored the settings-action branch August 20, 2018 20:03
@mkchoi212 mkchoi212 deleted the settings-action branch August 20, 2018 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants