Filetypes: use Layout API

[yourpalal]

Modify the Filetypes preference app to use the Layout API.

[stippi]

Thanks a lot for the patch! The code is mostly good, except for some hard to spot modifications you did which break some things. For example:

-	rect = topView->Bounds().InsetByCopy(8.0f, 8.0f);
-	fSignatureControl = new BTextControl(rect, "signature", "Signature:", NULL,
-		new BMessage(kMsgSignatureChanged), B_FOLLOW_LEFT_RIGHT);
-	fSignatureControl->SetModificationMessage(
+	//Signature
+	
+	fSignatureControl = new BTextControl("Signature:", NULL,
 		new BMessage(kMsgSignatureChanged));

The regular BMessage passed to a BControl is the so-called invokation message. It will be sent when the "value" of the control changes, for a BTextControl it means when the user pressed return or when the control loses focus. The modification message however will be send on each text change (each individual key-press of the user)!! So with this subtle change, you have completely altered the previous behavior, which some other part of the code probably relies on.

Another example of changing behavior is removing SetEnabled() calls:

@@ -471,12 +454,7 @@
 		fDescriptionView->SetText(NULL);
 	}
 
-	fNameView->SetEnabled(enabled);
-	fSignatureView->SetEnabled(enabled);
-	fPathView->SetEnabled(enabled);
 
-	fVersionView->SetEnabled(enabled);
-	fDescriptionLabel->SetEnabled(enabled);
 
 	fTrackerButton->SetEnabled(enabled && appFound);
 	fLaunchButton->SetEnabled(enabled && appFound);

The second big problem with the patch is that you have unfortunately not read our coding guidelines, and you didn't follow the existing code either, which was following the guidelines to the letter (most likely, since it was written by Axel and he is very strict with this stuff ;-).

So you need to go over the patch very carefully and look for places where you have perhaps changed the code behavior such that it breaks the preflet in subtle ways. And you need to fix the style issues. Mostly just a space after each comma and honoring the 80 chars per line limit. Some parts of the patch also change the indentation one level with no apparent reason.

[yourpalal]

thanks Stippi! I don't think I ever considered the difference in messages for a BTextControl, good catch there. I'm not sure why I removed those calls to SetEnabled() but I will fix that as well. I did try to follow the coding guidelines, but obviously I didn't do very well! I think what you might be mentioning for the sudden indentation is the code that uses LayoutBuilders, which I tried to follow the style used in this article ​http://www.haiku-os.org/documents/dev/laying_it_all_out_part_1 (you can see this by ctrl-f for Builder, if you're interested). Anyway, I will make those changes you've mentioned, do a thorough examination of the patch to find any other mistakes and put up a new patch, hopefully sometime this week, or on the weekend. Thanks again for reviewing the patch, your feedback is very helpful!

[stippi]

The layout builder indentation is fine. I mean parts of the patch like here:

@@ -151,23 +152,23 @@
 int
 SupportedTypeItem::Compare(const void* _a, const void* _b)
 {
-	const SupportedTypeItem* a = *(const SupportedTypeItem**)_a;
-	const SupportedTypeItem* b = *(const SupportedTypeItem**)_b;
+const SupportedTypeItem* a = *(const SupportedTypeItem**)_a;
+const SupportedTypeItem* b = *(const SupportedTypeItem**)_b;
 
-	int compare = strcasecmp(a->Text(), b->Text());
-	if (compare != 0)
-		return compare;
+int compare = strcasecmp(a->Text(), b->Text());
+if (compare != 0)
+	return compare;
 
-	return strcasecmp(a->Type(), b->Type());
+return strcasecmp(a->Type(), b->Type());
 }

[yourpalal]

Ah, I see, that is interesting, I'm not sure how that happened, I didn't mean to touch that code, I'll be sure to look through the patch and remedy such occurences! In the future I will definitely take more time to review my patches before I post them! Thanks for taking the time to find and point these things out!

[yourpalal]

added SetEnabled() stuff in ApplicationTypesWindow.cpp

[yourpalal]

Okay, there we are, I think that should do it, I've fixed the style violations (that I could find with the python tool), the modification vs. invocation message, the SetEnabled(), (I forgot to do ApplicationTypesWindow.cpp, but I've fixed that) I also did another window which I had missed the first time, and reviewed the patch, and it seems to be good. I've played around with the updated version next to the non-layout version and they seem to behave about the same. The one question I have is about be_control_look->GetDefaultItemSpacing(), which seems to return very large values, is it expected for an app to divide that number for certain uses? For the mean time, I've commented out the code that uses that data. Hopefully this patch looks good!

--Alex

[stippi]

Working on merging this patch with the changes from hrev36252.

[yourpalal]

Wow, thanks Stippi, I was planning on doing that, so if you would rather work on something else, I can do it. :D Thanks for all your help on this!

[yourpalal]

Haha, looks like you've already finished and committed this, so just ignore my comment above!

[stippi]

Yes, completely forgot to update the ticket status after checking it in. Applied in hrev36405. I figured it was kinda our fault for translating this app while completely breaking your patch. In any case thanks a bunch for your good work!

[axeld]

Fixed various layout problems, as well as the hard-coded spacing in hrev36882.