Created attachment 218373 [details] [review]
installer-media: Refactor label setup code

Created attachment 218374 [details] [review]
vm-creator: Append '-live' to live domains' names

Without this change, we end-up appending redundant numbers to titles of
live boxes, e.g 'Fedora (live) 2' even though there is no box by the
title 'Fedora (live)' but only 'Fedora'. Besides its better to be
consistent and keep name and title creation logic as close as possible.

Review of attachment 218373 [details] [review]:

Looks good apart from this live getter issue

::: src/installer-media.vala
@@ +115,3 @@
+        }
+
+        if (live)

'live' is:
public bool live { get { return os_media == null || os_media.live; } }
which looks wrong, the previous code makes more sense to me: os_media != null && os_media.live

Review of attachment 218374 [details] [review]:

Looks good. This made me wonder what we want to do with live medias after install? Wouldn't it be nicer to rename them to remove the (Live) from their name?

(In reply to comment #3)
> Review of attachment 218373 [details] [review]:
> 
> Looks good apart from this live getter issue
> 
> ::: src/installer-media.vala
> @@ +115,3 @@
> +        }
> +
> +        if (live)
> 
> 'live' is:
> public bool live { get { return os_media == null || os_media.live; } }
> which looks wrong, the previous code makes more sense to me: os_media != null
> && os_media.live

Well, 'os_media' can't be null if os is non-null so that part is redundant in in this context. Better to have check for 'live' in one place though hence is change.

(In reply to comment #4)
> Review of attachment 218374 [details] [review]:
> 
> Looks good. This made me wonder what we want to do with live medias after
> install? Wouldn't it be nicer to rename them to remove the (Live) from their
> name?

Good point, I'll look into that.

(In reply to comment #5)
> (In reply to comment #3)
> > Review of attachment 218373 [details] [review] [details]:
> > 
> > Looks good apart from this live getter issue
> > 
> > ::: src/installer-media.vala
> > @@ +115,3 @@
> > +        }
> > +
> > +        if (live)
> > 
> > 'live' is:
> > public bool live { get { return os_media == null || os_media.live; } }
> > which looks wrong, the previous code makes more sense to me: os_media != null
> > && os_media.live
> 
> Well, 'os_media' can't be null if os is non-null so that part is redundant in
> in this context.

But the getter does have a null check, so this explanation does not make the getter any better imo.


> Better to have check for 'live' in one place though hence is change.

I haven't said I disagree with this change, just that the getter looks wrong.

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Review of attachment 218373 [details] [review] [details] [details]:
> > > 
> > > Looks good apart from this live getter issue
> > > 
> > > ::: src/installer-media.vala
> > > @@ +115,3 @@
> > > +        }
> > > +
> > > +        if (live)
> > > 
> > > 'live' is:
> > > public bool live { get { return os_media == null || os_media.live; } }
> > > which looks wrong, the previous code makes more sense to me: os_media != null
> > > && os_media.live
> > 
> > Well, 'os_media' can't be null if os is non-null so that part is redundant in
> > in this context.
> 
> But the getter does have a null check, so this explanation does not make the
> getter any better imo.
> 
> 
> > Better to have check for 'live' in one place though hence is change.
> 
> I haven't said I disagree with this change, just that the getter looks wrong.

Yes in this context, one of the check is redundant. A redundant null check in one context is a very tiny price for any improvements to readability. IMO Keeping the logic (even though very simple one) in one place does improve readability since you can always just consult that line to know when we treat something as live and when not.

I said "wrong", not "suboptimal". When os_media is null, get_live will return 'true' while I'd expect 'false' to be returned, we have no media, how can we know if it's a live media or not? The duplicated code was returning false when there is no os_media, which made more sense even if the code was redundant.

(In reply to comment #9)
> I said "wrong", not "suboptimal". When os_media is null, get_live will return
> 'true' while I'd expect 'false' to be returned, we have no media,

Thats wrong expectation then. os_media being null means that its 'unknown media' (not 'no media') which we treat as live. Perhaps a comment saying that would help.

(In reply to comment #5)
> 
> Well, 'os_media' can't be null if os is non-null so that part is redundant in
> in this context. 

This is totally non obvious when looking through InstallerMedia, if you want to make that assumption, please move that change in a separate commit.

> Better to have check for 'live' in one place though hence is change.

Except that the 'live' getter isn't really checking that the media is a live media, it's 'live media or unknown', while in this context, we are exclusively interested in 'live media', so once again, another commit for that change if you really want it please. The second change would be

- if (os_media != null && os_media.live)
+ if (live)

and explain why this is legit to do, why we are sure we don't have a null os_media in this case, ...

(In reply to comment #11)
> (In reply to comment #5)
> > 
> > Well, 'os_media' can't be null if os is non-null so that part is redundant in
> > in this context. 
> 
> This is totally non obvious when looking through InstallerMedia, if you want to
> make that assumption, please move that change in a separate commit.
> 
> > Better to have check for 'live' in one place though hence is change.
> 
> Except that the 'live' getter isn't really checking that the media is a live
> media, it's 'live media or unknown', while in this context, we are exclusively
> interested in 'live media',

We are not exclusively interested in that, thats wrong way to put it. We are interested whether this is live case or not. Now if the getter does a redundant check that doesnt matter in this context, just let it! We are still getting the correct value from the getter.

> - if (os_media != null && os_media.live)
> + if (live)
> 
> and explain why this is legit to do, why we are sure we don't have a null
> os_media in this case, ...

We dont need to be sure, we are not assuming that. The legit reason is simplifying code here.

(In reply to comment #12)
> > Except that the 'live' getter isn't really checking that the media is a live
> > media, it's 'live media or unknown', while in this context, we are exclusively
> > interested in 'live media',
> 
> We are not exclusively interested in that, thats wrong way to put it. We are
> interested whether this is live case or not. 

get_live() really is get_live_or_unknown(), and in this case we are only interested knowing if it's live, I don't think we want the check to trigger if the media is unknown. That's what I meant by 'exclusively'.

> Now if the getter does a redundant
> check that doesnt matter in this context, just let it! We are still getting the
> correct value from the getter.
> 
> > - if (os_media != null && os_media.live)
> > + if (live)
> > 
> > and explain why this is legit to do, why we are sure we don't have a null
> > os_media in this case, ...
> 
> We dont need to be sure, we are not assuming that. The legit reason is
> simplifying code here.

It's a one-line test, so not really a big deal. Or do you mean it's not really important if unknown medias are marked as live here or not ?

(In reply to comment #13)
> (In reply to comment #12)
> > > Except that the 'live' getter isn't really checking that the media is a live
> > > media, it's 'live media or unknown', while in this context, we are exclusively
> > > interested in 'live media',
> > 
> > We are not exclusively interested in that, thats wrong way to put it. We are
> > interested whether this is live case or not. 
> 
> get_live() really is get_live_or_unknown(), and in this case we are only
> interested knowing if it's live, I don't think we want the check to trigger if
> the media is unknown. That's what I meant by 'exclusively'.

I understood what you meant but IMO you are looking at it from a wrong view. We just want to know whether its the case of live media or not (unknown media being treated as such as well).

> 
> > Now if the getter does a redundant
> > check that doesnt matter in this context, just let it! We are still getting the
> > correct value from the getter.
> > 
> > > - if (os_media != null && os_media.live)
> > > + if (live)
> > > 
> > > and explain why this is legit to do, why we are sure we don't have a null
> > > os_media in this case, ...
> > 
> > We dont need to be sure, we are not assuming that. The legit reason is
> > simplifying code here.
> 
> It's a one-line test, so not really a big deal. Or do you mean it's not really
> important if unknown medias are marked as live here or not ?

Its irralavent here cause in this context we know that media is known.

(In reply to comment #14)
 
> Its irralavent here cause in this context we know that media is known.

No we don't, or this relies on assumption on stuff outside of setup_for_path and the constructors.

(In reply to comment #15)
> (In reply to comment #14)
> 
> > Its irralavent here cause in this context we know that media is known.
> 
> No we don't, or this relies on assumption on stuff outside of setup_for_path
> and the constructors.

We can keep saying 'no, you are wrong' to each other if you like but that wont lead to anything.

Created attachment 218455 [details] [review]
InstallerMedia.from_iso_info doesn't have nullable params

None of the parameters of InstallerMedia.from_iso_info() have to be
nullable. Marking a non-nullable parameters as nullable could easily
lead to issues later.

Created attachment 218456 [details] [review]
installer-media: Refactor label setup code

V2: Make teuf happy :)

Review of attachment 218456 [details] [review]:

looks good, ack

Attachment 218374 [details] pushed as 85ba308 - vm-creator: Append '-live' to live domains' names
Attachment 218456 [details] pushed as db96a87 - installer-media: Refactor label setup code

now one more to go

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > 
> > > Its irralavent here cause in this context we know that media is known.
> > 
> > No we don't, or this relies on assumption on stuff outside of setup_for_path
> > and the constructors.
> 
> We can keep saying 'no, you are wrong' to each other if you like but that wont
> lead to anything.

You didn't even try to explain why it cannot be null, while I looked at the code and concluded otherwise, so not really my fault here if things are not moving forward...

(In reply to comment #22)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > 
> > > > Its irralavent here cause in this context we know that media is known.
> > > 
> > > No we don't, or this relies on assumption on stuff outside of setup_for_path
> > > and the constructors.
> > 
> > We can keep saying 'no, you are wrong' to each other if you like but that wont
> > lead to anything.
> 
> You didn't even try to explain

Perhaps I should refrain from trying as you don't even seem to notice/acknowledge that I do try. <sigh>

Attachment 218455 [details] pushed as 68fd4c8 - InstallerMedia.from_iso_info doesn't have nullable params

(In reply to comment #23)

> Perhaps I should refrain from trying as you don't even seem to
> notice/acknowledge that I do try. <sigh>

Sorry about that, but by 'explanation', I'm really thinking of quoting the code to show me that it cannot be NULL. The patch you just pushed is a step in that direction since it eliminates one of the ways it can be NULL. Then we have the call to
os = yield media_manager.os_db.guess_os_from_install_media (device_file, out os_media, cancellable);
in setup_for_path, which looks like it would return a NULL os_media on unknown media.
Basically, this could have been explained like "Media is marked as nullable in the constructor, but it's never NULL. I'll send a patch to change that. After the call to guess_os_from_install_media, os_media can only be NULL if os is NULL as well, so by the time we run the live check, we are sure we don't have an unknown media". You've been hacking on this code, so this kind of things is fresh on your mind, while it requires quite some digging to gather this information while reviewing the patch, so it doesn't hurt to explain them in details when things are not obvious to the reviewer, saves a lot of review time...