VLCDefaults

This MR is a proposal to remove direct use of UserDefaults, and instead access values through a wrapper VLCDefaults.

Pros:

• Allows simple dot notation to get and set values.
• Defaults which are accessed with composite keys use reliable functions instead of string building in various places in the codebase
• AppDelegate no longer needs the big dictionary in it
• Ownership is in Swift; Objective-C is a client instead of the other way around
• not included in this MR: objects can be serialized and deserialized reliably using swift Codable.
• not included in this MR: can be made observable, possibly even on a per-property basis.

Cons:

• Causes churn.
• No user facing benefit.
• Potential for mistakes during refactor

If this is wanted, I will continue.

changed the description

This is a very nice cleanup of the code base and I'd appreciate if you'd continue working on it! Please offer an ObjC API for your swift class, that's all I'd need.

added 69 commits

• 7e812d10...d6e5ccc8 - 59 earlier commits
• 5d533e0b - Fix a crash. Fix sorting of defaults keys.
• 19b8046b - Convert kVLCAudioLibraryGridLayout
• 1de3c31e - Convert kVLCAudioLibraryHideFeatArtists
• 1fcf4435 - Remove unused kVLCOptimizeItemNamesForDisplay
• eb3a3b5b - Convert kVLCIsCurrentlyPlayingPlaylist
• f7516a6b - Convert MediaLibraryDidForceRescan
• 2c951c46 - Minor cleanup.
• 62baffff - Convert kVLCHasLaunchedBefore
• 741f87b5 - Convert kVLCAudioLibraryHideTrackNumbers
• 5b248da4 - Remove some unused user default from objective C

Compare with previous version

Made a lot of progress, but still more to go.

added 18 commits

• 5b248da4...1e466ddb - 8 earlier commits
• f3e012cf - Convert kVLCHasActiveSubscription
• 3e18fa32 - Convert kVLCNumberOfLaunches
• 492efbe5 - Convert kVLCSortDescendingDefault
• 919395da - Convert kVLCSortDefault
• 60ad3d79 - Convert kVLCSettingPasscodeOnKey
• 531953f3 - Convert kVLCSettingDisableSubtitles
• 4e5a6c62 - Convert kVLCSettingsDecrapifyTitles. Reinstate title optimization code.
• 0f79a2f5 - Convert kVLCSaveDebugLogs
• 03640a2e - Remove preferenceKey from settings toggles.
• d3b35fe5 - Make VLCDefaults observable

Compare with previous version

added 92 commits

• 8fb3b470 - 1 commit from branch videolan:master
• 8fb3b470...885d4f63 - 81 earlier commits
• 60803cae - Convert kVLCSortDefault
• 06c67532 - Convert kVLCSettingPasscodeOnKey
• d5622a38 - Convert kVLCSettingDisableSubtitles
• bd032a01 - Convert kVLCSettingsDecrapifyTitles. Reinstate title optimization code.
• 94b7c2de - Convert kVLCSaveDebugLogs
• 6f176a15 - Remove preferenceKey from settings toggles.
• 4c8985dd - Make VLCDefaults observable
• 079b4666 - Add name to a few file headers
• ffc93c5b - Fix visionOS and tvOS builds.
• c5c18bee - Reinstate feature lost during recent rebase.

Compare with previous version

@fkuehne:

I have now completed the lion's share of the changes. One commit per migrated settings key. A few commits had minor mistakes, I believe I caught them all in the end in subsequent commits though.

The following are still outstanding:

A few classes in Objective-C still access defaults directly. They do not collide with the ones I have refactored though. They also use NSUbiquitousKeyValueStore, I'm wondering if there is some opportunity there to use this same pattern for that class (make our own wrapper and define our own properties). It might even be possible to define an interface that both wrapper classes share, and that code can talk to either one.

Settings which use an actionsheet to choose from multiple values still access defaults behind the new class' back. I think that part of settings needs some major attention anyway; it might be best for me to do a follow up MR where I address it holistically. I have some of this code marked deprecated, so the warnings list in Xcode has grown a bit here.

marked this merge request as ready

resolved all threads

There is a crash on startup during a cold install. It is a one line fix. I will submit it today.

Also, I found a number of (existing) mistakes in the Root.inApp.plist file.

I'm a bit conflicted on what to do about the issues I found in Root.inApp.plist.

1. tvOS is using a framework to manage settings, InAppSettingsKit.
2. It looks like at some point in the project's history, iOS broke away from this, but still uses the plist file for some settings.
3. The issues I have found are things like: values which are not of a consistent type, such as all integers, but one is a string instead. Some settings do not have a default value. Some settings are defined as an integer, but in code, they are used as a boolean. The app gets away with this because of how loose NSUserDefaults is with data types.
4. Root.plist has been consistently updated, including by me in the last months. But I blanked it out and all settings are still available. Is this file still needed?

I am wondering if it is worth divorcing the iOS app from the PLIST file entirely. It wouldn't be much work to define all of the settings in code, and use clearly defined data types and default values for each.

Thoughts?

1. correct
2. correct and to be honest, I believe that change was a mistake as the features provided by that framework were great and still are and the main reason for this change was that it was in ObjC and not swift. It took long to get the Settings to the current shape that is almost the same as with that framework.
3. Using an integer as a boolean is legal in C. However, of course, strings and integer are not legal to compare.
4. Root.plist is displayed in the Settings.app installed by default on iOS and I believe it is great to also show the options there.

ehm and yes, Root.inApp.list is a relict of the InAppSettingsKit time to differentiate between some options we only want to show within the app from those visible every where.

How about:

• define all iOS settings programmatically, finishing the divorce from IASK framework
• let tvOS continue with IASK and thus Root.inApp.plist. It can be maintained by consulting the iOS programmatic version to see what kind of data types are required, and legal values, etc.
• Root.plist can be treated similarly, with options not wanted in Settings.app removed.

Hard coding could look something like this:

struct Setting: Equatable {
    let id: ID
    let title: String
    let values: Values

    init(id: ID, title: String, values: Values) {
        self.id = id
        self.title = title
        self.values = values
    }

    var isValid: Bool {
        switch values {
        case let .bool(vals, defaultValue):
            return Self.validate(values: vals, defaultValue: defaultValue)
        case let .float(vals, defaultValue):
            return Self.validate(values: vals, defaultValue: defaultValue)
        case let .integer(vals, defaultValue):
            return Self.validate(values: vals, defaultValue: defaultValue)
        case let .string(vals, defaultValue):
            return Self.validate(values: vals, defaultValue: defaultValue)
        }
    }

    private static func validate<T: Equatable>(values: [T], defaultValue: T) -> Bool {
        !values.isEmpty && values.contains(defaultValue)
    }
}

extension Setting {
    enum Values: Equatable {
        case bool([Bool], defaultValue: Bool)
        case float([Float], defaultValue: Float)
        case integer([Int], defaultValue: Int)
        case string([String], defaultValue: String)
    }
}

extension Setting {
    enum ID: String, Equatable {
        case bassBoost = "bass-boost"
        case trebleBoost = "treble-boost"
    }
}

// MARK: - Settings

extension Setting {
    static var bassBoost: Setting {
        .init(
            id: .bassBoost,
            title: "Bass Boost",
            values: .bool([false, true], defaultValue: false)
        )
    }

    static var trebleBoost: Setting {
        .init(
            id: .trebleBoost,
            title: "Treble Boost",
            values: .integer([-1, 0, 1], defaultValue: 0)
        )
    }
}

What would be the benefit of doing that? Why not use the plist that was specifically invented by Apple for this purpose that is trivial to edit both using a text editor and the Xcode GUI?

The advantage to hard coding is that it would be basically impossible to have values of the incorrect type, or missing default values, and it would be easier to find orphan settings that don't exist anymore by using tools such as periphery.

But as I mentioned elsewhere, there's nothing wrong with sticking with PLIST as long as the existing problems can be found and fixed.

I'd prefer to fix the plist to be honest.

added 1 commit

• a767ab25 - Fix missing default playback speed, fix crash on fresh install. Fix some...

Compare with previous version

Appears to be unused, but it was in the PLIST.

No, there is a total black theme for OLED screens. If it is not accessible through the Settings, it's a bug. See the PresentationTheme.swift for reference.

I had a very hard time as a user and as a developer in finding this black background option. I had to choose the dark theme, which dismisses the appearance action sheet. Then, I had to open it again, and now there is a toggle switch for using a black background when in dark mode.

This paradigm might explain why the unreachable value in the PLIST didn't really matter.

I wonder if it is more logical to define an independent black theme, which happens to be very similar to the dark theme, but with the black background applied. Then, it does not need to be a composition of two different settings to achieve it. Was that how it worked in the past? Maybe that explains the 4th PLIST value being there.

The issue with that is that you might still want "Automatic" theme switching but still have the darker dark theme.

Maybe a reasonable solution would be to move the toggle switch for black background into the main settings UI, and have it appear or disappear based on whether it is relevant to the selected theme item right above it.

This would remove the requirement for the user to open the action sheet twice.

Yeah I agree, it's confusing it is in there. IMHO just make it separate thing right below the theme selection item and just disable it when it does not apply. That way its discoverable even when the user currently uses the light theme.

Fully agreed with Marvin here.

added 95 commits

• a767ab25...b7aad04b - 2 commits from branch videolan:master
• b7aad04b...d1193105 - 83 earlier commits
• 677f7a9f - Convert kVLCSettingDisableSubtitles
• 194de7c7 - Convert kVLCSettingsDecrapifyTitles. Reinstate title optimization code.
• 79ef1973 - Convert kVLCSaveDebugLogs
• b5bedb87 - Remove preferenceKey from settings toggles.
• 21fc89c1 - Make VLCDefaults observable
• 380433bb - Add name to a few file headers
• 8971bde4 - Fix visionOS and tvOS builds.
• 37eca7d7 - Reinstate feature lost during recent rebase.
• c0825e25 - Fix missing default playback speed, fix crash on fresh install. Fix some...
• ffe706a6 - MR feedback.

Compare with previous version

Regardless of existing threads, this is ready for review again.