Opened 7 years ago

Closed 7 years ago

#5525 closed enhancement (fixed)

Implement archiving in layouting classes

Reported by: mmadia Owned by: yourpalal
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 bonefish)

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

Download all attachments as: .zip

Change History (40)

comment:1 Changed 7 years ago by mmadia

  • Blocking 5524 removed

comment:1 Changed 7 years ago by mmadia

  • Blocking 5524 added
  • Component changed from Kits to Kits/Interface Kit
  • Owner changed from nobody to axeld

comment:2 Changed 7 years ago by bonefish

  • Description modified (diff)

comment:3 Changed 7 years ago by yourpalal

  • Blocked By 6256 added

comment:4 Changed 7 years ago by yourpalal

  • Blocked By 6257 added

comment:5 Changed 7 years ago by yourpalal

  • Blocked By 6302 added

comment:6 Changed 7 years ago by yourpalal

  • Blocked By 6314 added

Changed 7 years ago by yourpalal

implement archiving of BAbstractLayoutItem

comment:7 Changed 7 years ago by yourpalal

  • Has a Patch set

Changed 7 years ago by yourpalal

implement archiving of BTwoDimensionalLayout class

Changed 7 years ago by yourpalal

implement archiving of BGridLayout class.

Changed 7 years ago by yourpalal

implement archiving of BGridView class.

Changed 7 years ago by yourpalal

implement archiving of BGroupLayout class

Changed 7 years ago by yourpalal

implement archiving of BGroupView class

Changed 7 years ago by yourpalal

implement archiving of BSpaceLayoutItem class

Changed 7 years ago by yourpalal

implement archiving of BSplitLayout class

Changed 7 years ago by yourpalal

implement archiving of BSplitView class

Changed 7 years ago by yourpalal

implement archiving of BViewLayoutItem class.

comment:8 Changed 7 years ago by 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).

comment:9 Changed 7 years ago by pulkomandy

  • Owner changed from axeld to pulkomandy
  • Status changed from new to assigned

comment:10 Changed 7 years ago by bonefish

  • Owner changed from pulkomandy to bonefish
  • Status changed from assigned to in-progress

comment:11 follow-up: Changed 7 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 7 years ago by 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.

comment:13 Changed 7 years ago by 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.

comment:14 Changed 7 years ago by 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).

comment:15 Changed 7 years ago by bonefish

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

comment:16 Changed 7 years ago by 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.

comment:17 Changed 7 years ago by bonefish

BGroupView patch applied in hrev37546.

comment:18 Changed 7 years ago by 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.

comment:19 Changed 7 years ago by 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.

comment:20 follow-up: Changed 7 years ago by 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.

comment:21 Changed 7 years ago by 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.

comment:22 in reply to: ↑ 20 Changed 7 years ago by 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.

comment:23 Changed 7 years ago by 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!

Changed 7 years ago by yourpalal

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

Changed 7 years ago by yourpalal

implement archiving of BCardLayout class.

comment:24 Changed 7 years ago by bonefish

BSplitView patch applied in hrev37612.

comment:25 Changed 7 years ago by bonefish

BCardLayout patch applied in hrev37613.

comment:26 Changed 7 years ago by yourpalal

  • Owner changed from bonefish to yourpalal

comment:27 Changed 7 years ago by yourpalal

  • Resolution set to fixed
  • Status changed from in-progress to closed

I think this all finished after hrev37796.

Note: See TracTickets for help on using tickets.