Opened 8 years ago

Closed 7 years ago

#5525 closed enhancement (fixed)

Implement archiving in layouting classes

Reported by: Matt Madia Owned by: Alex
Priority: normal Milestone:
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: #6256, #6257, #6302, #6314 Blocking: #5524
Has a Patch: yes Platform: All

Description (last modified by Ingo Weinhold)

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.

Attachments (12)

abstractItem.patch (4.0 KB) - added by Alex 7 years ago.
implement archiving of BAbstractLayoutItem
2dlayout.patch (8.0 KB) - added by Alex 7 years ago.
implement archiving of BTwoDimensionalLayout class
gridLayout.patch (14.3 KB) - added by Alex 7 years ago.
implement archiving of BGridLayout class.
gridView.patch (2.1 KB) - added by Alex 7 years ago.
implement archiving of BGridView class.
groupLayout.patch (9.6 KB) - added by Alex 7 years ago.
implement archiving of BGroupLayout class
groupView.patch (1.9 KB) - added by Alex 7 years ago.
implement archiving of BGroupView class
spaceLayoutItem.patch (5.1 KB) - added by Alex 7 years ago.
implement archiving of BSpaceLayoutItem class
splitLayout.patch (15.3 KB) - added by Alex 7 years ago.
implement archiving of BSplitLayout class
splitView.patch (2.6 KB) - added by Alex 7 years ago.
implement archiving of BSplitView class
viewLayoutItem.patch (4.5 KB) - added by Alex 7 years ago.
implement archiving of BViewLayoutItem class.
splitView.2.patch (2.3 KB) - added by Alex 7 years ago.
Updated BSplitView archiving patch, which doesn't archive its layout.
cardLayout.patch (4.4 KB) - added by Alex 7 years ago.
implement archiving of BCardLayout class.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 8 years ago by Matt Madia

Blocking: 5524 removed

comment:1 Changed 8 years ago by Matt Madia

Blocking: 5524 added
Component: KitsKits/Interface Kit
Owner: changed from Nobody to axeld

comment:2 Changed 8 years ago by Ingo Weinhold

Description: modified (diff)

comment:3 Changed 7 years ago by Alex

Blocked By: 6256 added

comment:4 Changed 7 years ago by Alex

Blocked By: 6257 added

comment:5 Changed 7 years ago by Alex

Blocked By: 6302 added

comment:6 Changed 7 years ago by Alex

Blocked By: 6314 added

Changed 7 years ago by Alex

Attachment: abstractItem.patch added

implement archiving of BAbstractLayoutItem

comment:7 Changed 7 years ago by Alex

Has a Patch: set

Changed 7 years ago by Alex

Attachment: 2dlayout.patch added

implement archiving of BTwoDimensionalLayout class

Changed 7 years ago by Alex

Attachment: gridLayout.patch added

implement archiving of BGridLayout class.

Changed 7 years ago by Alex

Attachment: gridView.patch added

implement archiving of BGridView class.

Changed 7 years ago by Alex

Attachment: groupLayout.patch added

implement archiving of BGroupLayout class

Changed 7 years ago by Alex

Attachment: groupView.patch added

implement archiving of BGroupView class

Changed 7 years ago by Alex

Attachment: spaceLayoutItem.patch added

implement archiving of BSpaceLayoutItem class

Changed 7 years ago by Alex

Attachment: splitLayout.patch added

implement archiving of BSplitLayout class

Changed 7 years ago by Alex

Attachment: splitView.patch added

implement archiving of BSplitView class

Changed 7 years ago by Alex

Attachment: viewLayoutItem.patch added

implement archiving of BViewLayoutItem class.

comment:8 Changed 7 years ago by Alex

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).

comment:9 Changed 7 years ago by PulkoMandy

Owner: changed from axeld to PulkoMandy
Status: newassigned

comment:10 Changed 7 years ago by Ingo Weinhold

Owner: changed from PulkoMandy to Ingo Weinhold
Status: assignedin-progress

comment:11 Changed 7 years ago by Ingo Weinhold

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.

comment:12 in reply to:  11 Changed 7 years ago by Alex

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.

comment:13 Changed 7 years ago by Ingo Weinhold

... 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.

comment:14 Changed 7 years ago by Ingo Weinhold

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).

comment:15 Changed 7 years ago by Ingo Weinhold

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

comment:16 Changed 7 years ago by Ingo Weinhold

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.

comment:17 Changed 7 years ago by Ingo Weinhold

BGroupView patch applied in hrev37546.

comment:18 Changed 7 years ago by Ingo Weinhold

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.

comment:19 Changed 7 years ago by Ingo Weinhold

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.

comment:20 Changed 7 years ago by Ingo Weinhold

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.

comment:21 Changed 7 years ago by Ingo Weinhold

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.

comment:22 in reply to:  20 Changed 7 years ago by Alex

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.

comment:23 Changed 7 years ago by Alex

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!

Changed 7 years ago by Alex

Attachment: splitView.2.patch added

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

Changed 7 years ago by Alex

Attachment: cardLayout.patch added

implement archiving of BCardLayout class.

comment:24 Changed 7 years ago by Ingo Weinhold

BSplitView patch applied in hrev37612.

comment:25 Changed 7 years ago by Ingo Weinhold

BCardLayout patch applied in hrev37613.

comment:26 Changed 7 years ago by Alex

Owner: changed from Ingo Weinhold to Alex

comment:27 Changed 7 years ago by Alex

Resolution: fixed
Status: in-progressclosed

I think this all finished after hrev37796.

Note: See TracTickets for help on using tickets.