[haiku-commits] Re: r39115 - in haiku/trunk: headers/os/storage headers/private/storage src/add-ons/disk_systems/bfs src/add-ons/disk_systems/intel src/apps/drivesetup ...

  • From: Stephan Assmus <superstippi@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sun, 24 Oct 2010 10:50:39 +0200

Hi Bryce,

nice changes, some comments below. :-)

Am 24.10.2010 05:39, schrieb bgroff@xxxxxxxxxx:
Modified: haiku/trunk/headers/os/storage/DiskDeviceDefs.h
===================================================================
--- haiku/trunk/headers/os/storage/DiskDeviceDefs.h     2010-10-24 03:31:34 UTC 
(rev 39114)
+++ haiku/trunk/headers/os/storage/DiskDeviceDefs.h     2010-10-24 03:39:19 UTC 
(rev 39115)
@@ -148,6 +148,13 @@
        B_DISK_DEVICE_JOB_CAN_PAUSE                             = 0x08,
  };

+enum B_PARAMETER_EDITOR_TYPE {
+       B_CREATE_PARAMETER_EDITOR                               = 0x01,
+       B_INITIALIZE_PARAMETER_EDITOR                   = 0x04,
+       B_DELETE_PARAMETER_EDITOR                               = 0x08,
+       B_PROPERTIES_PARAMETER_EDITOR                   = 0x10
+};

The name of the type should be "BParameterEditorType", IMHO. Also since these are probably not flags, the values should be more like (0, 1, 2, 3). Or did you envision to have an app ask for "B_CREATE_PARAMETER_EDITOR | B_INITIALIZE_PARAMETER_EDITOR"?

Modified: haiku/trunk/src/apps/drivesetup/CreateParamsPanel.cpp
===================================================================
--- haiku/trunk/src/apps/drivesetup/CreateParamsPanel.cpp       2010-10-24 
03:31:34 UTC (rev 39114)
+++ haiku/trunk/src/apps/drivesetup/CreateParamsPanel.cpp       2010-10-24 
03:39:19 UTC (rev 39115)
@@ -265,9 +265,11 @@
                )
        );

-       parent->GetChildCreationParameterEditor(NULL,&fEditor);
-       if (fEditor)
+       status_t err = 
parent->GetParameterEditor(B_CREATE_PARAMETER_EDITOR,&fEditor);

missing space after comma

+       if (err == B_OK&&  fEditor != NULL)
                AddChild(fEditor->View());

weird spacing

+       else
+               fEditor = NULL;

        BButton* okButton = new BButton(B_TRANSLATE("Create"),
                new BMessage(MSG_OK));
@@ -281,3 +283,4 @@
        AddToSubset(fWindow);
        layout->View()->SetViewColor(ui_color(B_PANEL_BACKGROUND_COLOR));
  }


Modified: haiku/trunk/src/apps/drivesetup/InitParamsPanel.cpp
===================================================================
--- haiku/trunk/src/apps/drivesetup/InitParamsPanel.cpp 2010-10-24 03:31:34 UTC 
(rev 39114)
+++ haiku/trunk/src/apps/drivesetup/InitParamsPanel.cpp 2010-10-24 03:39:19 UTC 
(rev 39115)
@@ -16,6 +16,8 @@
  #include<Button.h>
  #include<Catalog.h>
  #include<ControlLook.h>
+#include<DiskSystemAddOn.h>
+#include<DiskSystemAddOnManager.h>
  #include<GroupLayout.h>
  #include<GroupLayoutBuilder.h>
  #include<Locale.h>
@@ -96,9 +98,21 @@

        fOkButton = new BButton(B_TRANSLATE("Initialize"), new 
BMessage(MSG_OK));

-       partition->GetInitializationParameterEditor(diskSystem.String(),
-               &fEditor);
+       DiskSystemAddOnManager* manager = DiskSystemAddOnManager::Default();
+       BDiskSystemAddOn* addOn = manager->GetAddOn(diskSystem);
+       if (addOn) {
+               // put the add-on
+               manager->PutAddOn(addOn);

(Despite my comments below...) Put the addon before using it? Isn't putting used for the reference count?

+               status_t err = 
addOn->GetParameterEditor(B_INITIALIZE_PARAMETER_EDITOR,&fEditor);
+               if (err != B_OK) {
+                       fEditor = NULL;
+               }
+       } else {
+               fEditor = NULL;
+       }
+

This change is wrong. The add-ons are supposed to be "hidden" from the API user. And DriveSetup is only supposed to use the "public" API.



Modified: haiku/trunk/src/apps/drivesetup/PartitionList.cpp
===================================================================
--- haiku/trunk/src/apps/drivesetup/PartitionList.cpp   2010-10-24 03:31:34 UTC 
(rev 39114)
+++ haiku/trunk/src/apps/drivesetup/PartitionList.cpp   2010-10-24 03:39:19 UTC 
(rev 39115)
@@ -221,7 +221,10 @@
                SetField(new BStringField(kUnavailableString), 
kFilesystemColumn);
                SetField(new BStringField(kUnavailableString), 
kVolumeNameColumn);
        } else {
-               SetField(new BStringField(partition->Type()), 
kFilesystemColumn);
+               if (partition->ContainsFileSystem())
+                       SetField(new BStringField(partition->Type()), 
kFilesystemColumn);
+               else
+                       SetField(new BStringField(kUnavailableString), 
kFilesystemColumn);              

But isn't the user interested in what type flag the partition has? He may be able to more easily recognize a partition by that.


Modified: haiku/trunk/src/kits/storage/disk_device/Partition.cpp
===================================================================
--- haiku/trunk/src/kits/storage/disk_device/Partition.cpp      2010-10-24 
03:31:34 UTC (rev 39114)
+++ haiku/trunk/src/kits/storage/disk_device/Partition.cpp      2010-10-24 
03:39:19 UTC (rev 39115)
@@ -1083,13 +1083,13 @@

  // GetParameterEditor
  status_t
-BPartition::GetParameterEditor(BPartitionParameterEditor** editor)
+BPartition::GetParameterEditor(B_PARAMETER_EDITOR_TYPE type,
+       BPartitionParameterEditor** editor)
  {
-       BPartition* parent = Parent();
-       if (!parent || !fDelegate)
+       if (!fDelegate)
                return B_NO_INIT;

-       return parent->fDelegate->GetParameterEditor(fDelegate, editor);
+       return fDelegate->GetParameterEditor(type, editor);
  }

This may point to a problem in the change overall, which is who is responsible for what. It looks like you would have to check the type here already and depending on this, ask either the delegate of the parent, or your own delegate.


Modified: haiku/trunk/src/kits/storage/disk_device/PartitionDelegate.cpp
===================================================================
--- haiku/trunk/src/kits/storage/disk_device/PartitionDelegate.cpp      
2010-10-24 03:31:34 UTC (rev 39114)
+++ haiku/trunk/src/kits/storage/disk_device/PartitionDelegate.cpp      
2010-10-24 03:39:19 UTC (rev 39115)
@@ -8,10 +8,8 @@
  #include<stdio.h>
[...]
  // SetContentParameters
  status_t
  BPartition::Delegate::SetContentParameters(const char* parameters)
@@ -425,6 +398,12 @@
  bool
  BPartition::Delegate::CanInitialize(const char* diskSystem) const
  {
+       // HACK TO HELP BLANK PARTITION'S BECOME INITIALIZED.
+       if (diskSystem == NULL)
+               return true;
+       if (strlen(diskSystem)<  1)
+               return true;
+
        // get the disk system add-on
        DiskSystemAddOnManager* manager = DiskSystemAddOnManager::Default();
        BDiskSystemAddOn* addOn = manager->GetAddOn(diskSystem);

This looks really weird and in conflict with the intentions of how things are supposed to work. What's wrong with checking whether an add-on is available that supports the operation?

Best regards,
-Stephan

Other related posts: