fix(theming): only insert default theme if it's still used

[devversion]

Currently if I specify a default theme by using #setDefaultTheme, then the default theme will be still inserted.

Fixes #7425

[EladBezalel]

!(theme.name !== $mdTheming.defaultTheme() && theme.name === 'default')

is equals to

theme.name !== 'default'

[devversion]

No, that's wrong. Imagine you have set the default theme to myDefaultTheme
And it's iterating through the themes, and their is default now in progress.

theme.name !== 'default'
// 'default' !== 'default' === FALSE

So in this case it works.

But now, if you have no default theme set, then it will never insert the default theme and the theming breaks.

[EladBezalel]

it's just that

!(theme.name !== $mdTheming.defaultTheme() && theme.name === 'default')

doesn't makes sense, you are checking if theme.name isn't equals to something AND equals to something else, if you know that you expect it to be default than why do you even bother to check if it's not equals to something else

[devversion]

Oh it makes sense. An if-expression won't continue the check if it already failed.
For example:

true && false && true
-> The check will immediately cancel itself, because the second `AND` is false.

The theme should be skipped if the theme name is equal to default and if the default theme has been changed.

But maybe we can find a more elegant check, but your logic in the check isn't valid.

[EladBezalel]

I'm trying to figure when your addition will be valid,

!(theme.name !== $mdTheming.defaultTheme() && theme.name === 'default')

let's say theme.name !== $mdTheming.defaultTheme() is true, than for sure theme.name === 'default' is false. - invalid
theme.name !== $mdTheming.defaultTheme() is false and theme.name === 'default' is false (or true) - invalid

it will only be valid if both are true which means $mdTheming.defaultTheme() !== 'default'

[devversion]

Let's go through a few cases.

Case 1

We have the following mocked theme array

THEMES = ['default', ' blue']

And my default theme is set to blue

So the iteration will check:

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`
// Negate the check, so if both conditions are true, 
// then the theme won't be inserted into the DOM
if (!(
  themeName !== 'blue' // true -> continue with the checks `AND`
  && themeName === 'default' // true -> both conditions are true
)

So in this case, only the theme blue will inserted into the DOM - Valid

Case 2

We have the following mocked theme array

THEMES = ['default', ' blue']

And my default theme is set to default (default behavior)

So the iteration will check:

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`
// Negate the check, so if both conditions are true, 
// then the theme won't be inserted into the DOM
if (!(
  themeName !== 'default' // false -> cancel check and and return false
  && themeName === 'default' // Not reachable
)
// The check will pass, because of the negated expression 
// !(false) ==> true

So in this case, both themes will be inserted to the DOM

[EladBezalel]

First of all let's use discrete to simplify the conditions

!(a != z && b == z) 
// => 
(a == z || b != z)

Ok so let's go through your cases.

Case 1

We have the following mocked theme array

THEMES = ['default', ' blue']

And my default theme is set to blue

So the iteration will check:

// Replicates the iteration - case for `default
var themeName = THEMES[0]; // `default`
// if one conditions is true, the theme won't be inserted into the DOM
if (themeName === 'blue' // false
  || themeName !== 'default' // false -> both conditions are false
)

So in this case, only the theme blue will inserted into the DOM - Valid

Case 2

We have the following mocked theme array

THEMES = ['default', ' blue']

And my default theme is set to default (default behavior)
So the iteration will check:

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`
// if one conditions is true, the theme won't be inserted into the DOM
if (themeName === 'default' // true
  || themeName !== 'default' //  false
)
// The check will pass

So in this case, both themes will be inserted to the DOM

Finally

As you can see, in both cases

themeName !== 'default' //  false

Than it's clearly redundant..

[devversion]

But imagine, as I said these are examples have a custom default theme set. So if you haven't changed the default theme, but you registered another theme.

So now take a look at Case 2. If we don't check both, as in mine implementation, then you'll have the following execution.

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`

// This is the check, which can be simplified in your opinion.
if (themeName !== 'default' // FALSE
)
// The check won't pass

So this case won't be valid. Because here 'default' is our default theme and won't be injected into the DOM, because the check didn't pass.

We won't add the iterated theme when:
-The default theme has been changed by the user

• And the iterated theme is 'default

[EladBezalel]

I don't say you should use:

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`

// This is the check, which can be simplified in your opinion.
if (themeName !== 'default' // FALSE
)
// The check won't pass

i say you should use:

// Replicates the iteration - case for `default`
var themeName = THEMES[0]; // `default`
// if one conditions is true, the theme won't be inserted into the DOM
if (themeName === 'default') // theme.name === $mdTheming.defaultTheme()

[ThomasBurleson]

@devversion, @EladBezalel - what is the status here? It appears that Elad's logic is valid. Will it pass all the test cases ?

[devversion]

@ThomasBurleson We're still discussing.

I mean Elad says that, I should use this code:

if (themeName === 'default') // theme.name === $mdTheming.defaultTheme()

But isn't valid. Imagine if the user changed the default theme, then it's no longer default.

[ThomasBurleson]

tl;dr
@devversion - what is the status of this PR?

[devversion]

@ThomasBurleson We never had our dicussion. @EladBezalel Let's talk about that soon okay?

[devversion]

Closing this, as we both have different opinions about the logic. Let's revisit this anytime later.