Created attachment 223485 [details] [review]
Show name column even if we can't load column preferences

This is a bit better than crashing.

Review of attachment 223485 [details] [review]:

::: src/nautilus-list-view.c
@@ +1245,3 @@
+	if (column_order != NULL) {
+		all_columns = nautilus_sort_columns (all_columns, column_order);
+	}

This shouldn't be necessary, as nautilus_sort_columns() does the NULL check already.

@@ +1259,3 @@
+		}
+	} else {
+		g_hash_table_insert (visible_columns_hash, g_strdup ("name"), g_strdup ("name"));

I'd rather not have this here...nothing bad should happen if we leave the HT empty, no?

Review of attachment 223485 [details] [review]:

::: src/nautilus-list-view.c
@@ +1245,3 @@
+	if (column_order != NULL) {
+		all_columns = nautilus_sort_columns (all_columns, column_order);
+	}

It does the null check wrong :)

It should probably return the list instead of NULL when the input is NULL.

@@ +1259,3 @@
+		}
+	} else {
+		g_hash_table_insert (visible_columns_hash, g_strdup ("name"), g_strdup ("name"));

Rather have what? If the hash table is empty you open a blank window with no columns. That seems pretty horrible.

(In reply to comment #3)
> Review of attachment 223485 [details] [review]:
> 
> ::: src/nautilus-list-view.c
> @@ +1245,3 @@
> +    if (column_order != NULL) {
> +        all_columns = nautilus_sort_columns (all_columns, column_order);
> +    }
> 
> It does the null check wrong :)
> 
> It should probably return the list instead of NULL when the input is NULL.

Okay, that can be changed then.

> @@ +1259,3 @@
> +        }
> +    } else {
> +        g_hash_table_insert (visible_columns_hash, g_strdup ("name"), g_strdup
> ("name"));
> 
> Rather have what? If the hash table is empty you open a blank window with no
> columns. That seems pretty horrible.

The thing is we should never reach this code with a NULL value, unless the user forcefully disabled all the columns in the preferences or we hit a GSettings bug (in the stacktrace we're being called with a value coming directly from GSettings, and the default in the schema is not null).
We don't make one column mandatory anywhere else (e.g. you can remove all the columns from Visible Columns), so I don't think we should do it here, at least not as part of this fix.

I don't think we should allow disabling the name column ever. And I think even in the case of a gsettings bug coming up with no columns is unacceptable. The entire point of this patch is to guard against a broken gsettings basically and I think that simply not crashing but coming up unusable isn't worth doing.

(In reply to comment #5)
> I don't think we should allow disabling the name column ever. And I think even
> in the case of a gsettings bug coming up with no columns is unacceptable. The
> entire point of this patch is to guard against a broken gsettings basically and
> I think that simply not crashing but coming up unusable isn't worth doing.

I agree with you that coming up with an empty view is not something that we want.
However, that still sounds like a more general problem to me, which involves changing the Visible Columns dialog too. Until we enforce that consistently in all the situations, I'd rather take the simplest possible fix for this crasher.

Created attachment 223542 [details] [review]
Show name column even if we can't load column preferences

This is a bit better than crashing.

Review of attachment 223542 [details] [review]:

::: src/nautilus-list-view.c
@@ +1260,1 @@
 	}

As I said in the previous comment, I don't think this is the right place to fix it. I'd rather take only the other parts of the patch, and fix the empty view separately.

Created attachment 223566 [details] [review]
Don't crash if we can't load column preferences

Review of attachment 223566 [details] [review]:

Thanks!

Attachment 223566 [details] pushed as 185dad3 - Don't crash if we can't load column preferences