Implement archiving in layouting classes

[mmadia]

Documenting this & leaving the milestone open for now.

As mentioned on this ​haiku-development mailing list thread, archiving a BView hierarchy is not yet implemented. This is one of the features needed before the layout API can become public.

The BLayout hierarchy mirrors and extends the BView hierarchy. The unarchiving process has to map those to each other again. Particularly nasty is the BTwoDimensionalLayout::AlignLayoutWith() feature, which introduces cross-references between (sub-)hierarchies.

[yourpalal]

implement archiving of BAbstractLayoutItem

[yourpalal]

implement archiving of BTwoDimensionalLayout class

[yourpalal]

implement archiving of BGridLayout class.

[yourpalal]

implement archiving of BGridView class.

[yourpalal]

implement archiving of BGroupLayout class

[yourpalal]

implement archiving of BGroupView class

[yourpalal]

implement archiving of BSpaceLayoutItem class

[yourpalal]

implement archiving of BSplitLayout class

[yourpalal]

implement archiving of BSplitView class

[yourpalal]

implement archiving of BViewLayoutItem class.

[yourpalal]

The patches I've added cannot be applied until the patch on #6256 is. These patches can be reviewed & applied separately, in any order (at least I'm pretty sure :P).

[bonefish]

BAbstractLayoutItem patch applied in hrev37540. Given that each field name for the sizes takes more space than all size data together, it might be a good idea to store them in a single field. Same in BView, where I actually saw that this is how it is done for colors and font properties.

[yourpalal]

Replying to bonefish:

BAbstractLayoutItem patch applied in hrev37540. Given that each field name for the sizes takes more space than all size data together, it might be a good idea to store them in a single field. Same in BView, where I actually saw that this is how it is done for colors and font properties.

Yeah, that is a good idea, I'll do that soon.

[bonefish]

... forgot to mention: I also inserted missing blank lines in AbstractLayoutItem.cpp.

Applied the BTwoDimensionalLayout patch in hrev37541, though with the following style changes:

@@ -197,10 +216,10 @@

 	// implementation private
 private:
-			BTwoDimensionalLayout*	fLayout;
-			CompoundLayouter*	fHLayouter;
-			VerticalCompoundLayouter* fVLayouter;
-			BList				fHeightForWidthItems;
+			BTwoDimensionalLayout*		fLayout;
+			CompoundLayouter*			fHLayouter;
+			VerticalCompoundLayouter* 	fVLayouter;
+			BList						fHeightForWidthItems;

Though a bit ugly, this was actually (almost) how it is supposed to look. Where the type is too wide it is simple separated by a space from the variable name.

+BTwoDimensionalLayout::BTwoDimensionalLayout(BMessage* from)
+	:
+	BLayout(from),
+	fLeftInset(0),
+	fRightInset(0),
+	fTopInset(0),
+	fBottomInset(0),
+	fHSpacing(0),
+	fVSpacing(0),
+	fLocalLayouter(new LocalLayouter(this))
+{
+	float LeftInset;
+	float RightInset;
+	float TopInset;
+	float BottomInset;
+
+
+	from->FindFloat(kHSpacingField, &fHSpacing);
+	from->FindFloat(kVSpacingField, &fVSpacing);
+}

Lower camel case. Only one blank line.

+status_t
+BTwoDimensionalLayout::LocalLayouter::AlignLayoutsFromArchive(
+	BUnarchiver* unarchiver, orientation posture)
+{
+	const char* field = kHAlignedLayoutField;
+	if (posture == B_VERTICAL)
+		field = kVAlignedLayoutField;
+
+	int32 count;
+	status_t err = unarchiver->ArchiveMessage()->GetInfo(field, NULL, &count);
+	if (err == B_NAME_NOT_FOUND)
+		return B_OK;
+
+	BTwoDimensionalLayout* retriever;
+	for (int32 i = 0; i < count && err == B_OK; i++) {
+		err  = unarchiver->FindObject(field, i,

Superfluous space.

Another remark (I didn't not change anything here):

+status_t
+BTwoDimensionalLayout::CompoundLayouter::AddAlignedLayoutsToArchive(
+	BArchiver* archiver, LocalLayouter* requestedBy)
+{
+	// The LocalLayouter* that really owns us is at index 0, layouts
+	// at other indices are aligned to this one.
+	if (requestedBy != fLocalLayouters.ItemAt(0))
+		return B_OK;
+
+	status_t err;
+	for (int32 i = fLocalLayouters.CountItems() - 1; i > 0; i--) {
+		LocalLayouter* layouter = (LocalLayouter*)fLocalLayouters.ItemAt(i);
+
+		bool wasAvailable;
+		err = layouter->AddOwnerToArchive(archiver, this, wasAvailable);
+		if (err != B_OK && wasAvailable)
+			return err;
+	}
+	return B_OK;
+}

The wasAvailable parameter seems superfluous to me. AddOwnerToArchive() could simply return B_OK, if not available.

Also, similar to the BAbstractLayoutItem sizes, the insets and the spacing fields could be joined respectively.

[bonefish]

BGridLayout patch applied in hrev37542. A comment:

@@ -21,11 +23,25 @@
 	MAX_COLUMN_ROW_COUNT	= 1024,
 };

-// a placeholder we put in our grid array to make a cell occupied
-static BLayoutItem* const OCCUPIED_GRID_CELL = (BLayoutItem*)0x1;
+namespace {
+	// a placeholder we put in our grid array to make a cell occupied
+	BLayoutItem* const OCCUPIED_GRID_CELL = (BLayoutItem*)0x1;

+	const char* kRowCountField = "gridlayout:rowcount";
+	const char* kRowMinSizeField = "gridlayout:rowminsize";
+	const char* kRowMaxSizeField = "gridlayout:rowmaxsize";
+	const char* kRowWeightField = "gridlayout:rowweight";
+	const char* kColumnCountField = "gridlayout:columncount";
+	const char* kColumnMinSizeField = "gridlayout:columnminsize";
+	const char* kColumnMaxSizeField = "gridlayout:columnmaxsize";
+	const char* kColumnWeightField = "gridlayout:columnweight";
+	const char* kItemHeight = "gridlayout:item:height";
+	const char* kItemWidth = "gridlayout:item:width";
+	const char* kItemX = "gridlayout:item:x";
+	const char* kItemY = "gridlayout:item:y";
+}

For the last four constants you break the naming scheme, omitting the "Field" suffix. And BTW, for those to actually be constants their type would need to be const char* const -- not that this isn't a common mistake (... I'm sure I have often made in the past, too).

[bonefish]

BGridView patch applied in hrev37543 with some blank lines added/removed.

[bonefish]

Forgot to mention that for BGridLayout the message field grouping could be used to save space as well. In case that applies to more classes, I'm sure you'll find them all. :-)

Applied the BGroupLayout patch in hrev37545 with some blank line adjustments plus changes addressing the following:

+#include <new>
+using std::nothrow;

Please rather use std::nothrow in the code (same for std::string, std::map, etc.). I believe only old code exclusively written for gcc 2 and adjusted for gcc 4 with minimal effort uses the the using directives. Where it gets longish (e.g. std::map<std::string, std::string>) a typedef helps.

+BGroupLayout::BGroupLayout(BMessage* from)
+	:
+	BTwoDimensionalLayout(from)
+{
+	bool isVertical;
+	if (from->FindBool(kVerticalField, &isVertical) != B_OK)
+		isVertical = false;
+	fOrientation = (isVertical) ? B_VERTICAL : B_HORIZONTAL;

Superfluous parentheses.

[bonefish]

BGroupView patch applied in hrev37546.

[bonefish]

Applied the BSpaceLayoutItem patch in hrev37547 with some style corrections, most notably:

Index: src/kits/interface/SpaceLayoutItem.cpp
===================================================================
--- src/kits/interface/SpaceLayoutItem.cpp	(revision 37523)
+++ src/kits/interface/SpaceLayoutItem.cpp	(working copy)
@@ -1,29 +1,58 @@
 /*
+ * Copyright 2010, Haiku, Inc.
  * Copyright 2006, Ingo Weinhold <bonefish@cs.tu-berlin.de>.
  * All rights reserved. Distributed under the terms of the MIT License.
  */

+#include <new>
+
+#include <Message.h>
 #include <SpaceLayoutItem.h>

The header corresponding to the source file should be the first one (and separated). This makes sure the header is self-contained.

 void
-BSpaceLayoutItem::SetFrame(BRect frame)
-{
+BSpaceLayoutItem::SetFrame(BRect frame) {

Beep. In case you wonder why some methods used this incorrect style, those were probably left-overs. I originally prototyped the layout functionality in Java and just transcribed most of it to C++ afterwards. That's also the reason for BLayout::Item{Added,Removed} not having gotten a return type, since memory allocation issues are reported via exception in Java.

[bonefish]

Applied the BSplitLayout patch in hrev37548. Remarks:

+// archivng constants
+namespace {
+	const char* kInfoCollapsibleField = "BSplitLayout::info::collapsible";
+	const char* kInfoWeightField = "BSplitLayout::info::weight";
+	const char* kSpacingField = "BSplitLayout::spacing";
+	const char* kSplitterSizeField = "BSplitLayout::splitterSize";
+	const char* kIsVerticalField = "BSplitLayout::vertical";
+	const char* kTopInsetField = "BSplitLayout::TopInset";
+	const char* kBottomInsetField = "BSplitLayout::BottomInset";
+	const char* kLeftInsetField = "BSplitLayout::LeftInset";
+	const char* kRightInsetField = "BSplitLayout::RightInset";
+}

I noticed, the naming of the fields varies a lot between the classes (real class name or not, one or two colons as separator, case). Even just here the case is inconsistent.

 void
 BSplitLayout::_LayoutItem(BLayoutItem* item, ItemLayoutInfo* info)
 {
@@ -806,6 +939,7 @@

 	item->AlignInFrame(info->layoutFrame);

+	// TODO: shouldn't this be done (as needed) by item->SetVisible();
 	// if the item became visible, we need to update its internal layout
 	if (visibilityChanged) {
 		if (BView* itemView = item->View())

I removed the TODO, but good question. I believe in this case the deal is that an immediate relayout of the view is desired. item->SetVisible(), which for BViewLayoutItems just calls BView::Show(), would trigger the relayout as well, but asynchronously. The relayout could therefore happen after potentially already pending mouse moved events, which might confuse the split layout. I have to admit the workings of the split view/layout are rather complicated and have weird dependencies and side effects (and I don't pretend to still know them all). I'd be very careful with changing something and also check more obscure setups including nested split views with height-for-width items.

[bonefish]

I didn't apply the BSplitView patch. The layout is archived and unarchived by BView anyway. Having another field just to save a cast (which has to be done anyway) seems rather pointless.

[bonefish]

Applied the BViewLayoutItem patch in hrev37549 with the following change:

+#include <Layout.h>
 #include <View.h>

+#include <new>

Move the more generic <new> include before the block with the Haiku headers.

[yourpalal]

Replying to bonefish:

I didn't apply the BSplitView patch. The layout is archived and unarchived by BView anyway. Having another field just to save a cast (which has to be done anyway) seems rather pointless.

Good point, I'll re-work that.

[yourpalal]

Thanks, Ingo, for all the style help, review, pointers etc. I'll clean up the archiving field names & strings & combine variables into arrays soon and get a patch up!

[yourpalal]

Updated BSplitView archiving patch, which doesn't archive its layout.

[yourpalal]

implement archiving of BCardLayout class.

[bonefish]

BSplitView patch applied in hrev37612.

[bonefish]

BCardLayout patch applied in hrev37613.

[yourpalal]

I think this all finished after hrev37796.