Re: BDiskSystem::Supports*()/Validate*()

"Ingo Weinhold" <bonefish-CFLBMwTPW48UNGrzBIF7/[email protected]> wrote:
[...]
> > rarely needed, I think it would be a good idea to have the 
> > ContentSize() feature in there if we're going to support 
> > non-destructive enlarging even when the contents of the partition 
> > cannot (and are not) enlarged.
> OK, it's no problem to add it. The question is then, whether we want 
> to 
> allow resizing the contents individually 
> (BPartition::ResizeContents(off_t)), or at least allow to pass both 
> size and content size to Resize():
> 
> 	status_t Resize(off_t size, off_t contentSize);
> 
> CanResize() and ValidateResize() would need to be adjusted 
> accordingly. 
> Against separating the features speaks, that it's more work for the 
> API 
> user, since they must take care of the order of applying the resize 
> operations (contents, partition when shrinking, the other way around 
> when enlarging).

I think it would be okay if contentSize had a default parameter in 
which case it'll be the same as size. OTOH I don't think those changes 
are worth the effort, since I cannot really imagine a situation where 
you want to do that.

> > > True enough, but it would always create the need to have another
> > > variable and check, even if what I want to know would be this:
> > > bool canResizeContents;
> > > if (CanResize(&canResizeContents) && canResizeContents) {
> > >     ...
> > > }
> > > 
> > > I think it would be clearer to write:
> > > if ((ResizeCapabilities() & (CAN_RESIZE | CAN_RESIZE_CONTENTS)) !
> > > = 
> > > 0) 
> > > {
> > >     ...
> > > }
> Unfortunately not having the same semantics though. :-P
> It would correctly read:
> if (!(~ResizeCapabilities() & (CAN_RESIZE | CAN_RESIZE_CONTENTS))) {
> 	...
> }
> (significantly longer constant names BTW ;-)
> 
> Looking less sexy I have to admit.

Oh, yes, I see.

> > > But if the contents of the partition are supposed to be going 
> > > away
> > > anyway, you could also just check:
> > > if (CanResize()) {
> > >     ...
> > > }
> > > without having to define an extra boolean parameter.
> That would work with the current version as well, when providing a 
> default parameter for `canResizeContents'. I didn't, because I 
> assumed 
> that one would always be interested in the full truth, but it's no 
> big 
> deal to change that.

Perhaps we should do that, then?

> > > OTOH for DriveSetup its use would probably nice, I have to admit:
> > > 
> > > bool canResizeContents;
> > > if (CanResize(&canResizeContents)) {
> > >     if (!canResizeContents)
> > >         warnUserAndAbortIfHeWantsTo();
> > > 
> > >     // resize the partition
> > >     ...
> > > }
> Yep.

Since DriveSetup is the main idea behind this API, maybe we just ditch 
my ResizeCapabilities() idea again, and keep what we have now?

> > > True, CanResize() wouldn't be needed anymore - yet the value it 
> > > returns is as confusing as in the current solution (IMO).
> > If we don't need CanResize(), then why not ditch it? The only 
> > problem 
> > I can see with ResizeCapabilities is that is doesn't match the 
> > CanXYZ() 
> > naming scheme.
> Yes, that's pretty much what I think too.

I could also undo my suggestions, and we stay with what we have - since 
it seems well suited for the use in DriveSetup and similar apps.

> > I'm not sure I see what would be gained by splitting CanMove() into 
> > two 
> > functions, though. If you really want to be able to get just the 
> > info 
> > returned by CanMove(), why not allow NULL to be passed for the 
> > BObjectLists and provide default parameters?
> Same here, no big deal to do that.

Yes, that would be nice.

Adios...
   Axel.