Opened 7 years ago

Closed 7 years ago

#8872 closed enhancement (fixed)

Add ability to drag drive to terminal

Reported by: X512 Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/DriveSetup Version: R1/Development
Keywords: Cc: jwlhc172@…
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

After dropping, path to device (/dev/disk/...) should be pasted to terminal. It will be useful for commands like «dd». Also Cmd+C should copy path to selected device to clipboard.

Attachments (4)

0001-Added-ability-to-drag-drive-partition-from-list-to-t.patch (2.7 KB ) - added by jwlh172 7 years ago.
Patch with drag and drop functionality.
0002-Added-drag-drop-of-drive-partition-paths-drag-row-in.patch (2.6 KB ) - added by jwlh172 7 years ago.
Additional patch; to be applied following 0001.
0003-Corrected-code-spacing-formatting-issues.-8872.patch (1.3 KB ) - added by jwlh172 7 years ago.
* Forgot to add spaces after if statements.
0001-Added-drag-drop-of-disk-partition-paths-drag-a-list-.patch (3.1 KB ) - added by jwlh172 7 years ago.
New patch; replaces all above patches; all together in this one.

Download all attachments as: .zip

Change History (13)

by jwlh172, 7 years ago

Patch with drag and drop functionality.

comment:1 by jwlh172, 7 years ago

Has a Patch: set

comment:2 by jwlh172, 7 years ago

I've created a patch adding requested drag and drop functionality. I've also begun work on a Cmd+C solution, though the addition of drag and drop was much less intrusive than the clipboard addition, so I'm holding off on that one for now. Besides, Haiku being a very visual OS, I figured to begin with the drag and drop would be more important/useful.

Last edited 7 years ago by jwlh172 (previous) (diff)

comment:3 by stippi, 7 years ago

Patch looks fine in terms of functionality. You can improve the code a bit in terms of simplifying it and making it more coding style compliant:

BRow* _selectedRow = RowAt(rowPoint); 
if(_selectedRow) { 
    PartitionListRow* selectedRow 
        = dynamic_cast<PartitionListRow*>(_selectedRow); 

You can simplify that as:

   PartitionListRow* selectedRow 
       = dynamic_cast<PartitionListRow*>(RowAt(rowPoint));
   if (selectedRow != NULL) {

It is safe to pass NULL to dynamic_cast. In addition, you also catch the case where the returned BRow is in fact not of type PartitionListRow in which case dynamic_cast returns NULL which the original did not check for. BTW, I would call the variable "draggedRow" instead of "selectedRow". Makes it clearer, IMHO.

The same change is applicable for the BStringColumn retrival. Also note the space after "if".

What I don't like so much about the implementation is how it mixes model specific knowledge with view specific knowledge. But it's been very long since I looked at or even wrote the code, so it may perfectly fit with the rest of PartitionListView.cpp, I don't remember. At the same time I am not a fan of making things overlay complicated. But if there is an easy alternative to your solution that better separates the model from the view logic, then I'd prefer that. (The list view would forward the event to the model class, if there even is such a thing, and for example let it fill out the contents of the drag BMessage.)

Thanks for working on this!

comment:4 by leavengood, 7 years ago

This file does not seem particularly MVC to me, so I think the implementation in the patch is fine.

comment:5 by jwlh172, 7 years ago

Cc: jwlhc172@… added

by jwlh172, 7 years ago

Additional patch; to be applied following 0001.

by jwlh172, 7 years ago

  • Forgot to add spaces after if statements.

comment:6 by jwlh172, 7 years ago

I've simplified the code where possible and, in an attempt to adhere at least a bit more to MVC, I've added public method GetPath() to the PartitionListRow class, so now I simply call that method from my PartitionListView in order to receive the associated drive/partition path.

comment:7 by stippi, 7 years ago

Thanks for the update. Sorry, I've got more improvements to suggest:

const char* 
PartitionListRow::GetPath()
{ 
    char* pathString = NULL;

    BBitmapStringField* stringField
        = dynamic_cast<BBitmapStringField*>(GetField(kDeviceColumn));
    if(stringField != NULL) {
        pathString = const_cast<char*>(stringField->String());
    }

    return pathString;
}

According to Haiku's conventions, the method should be named just "Path()". Maybe "DevicePath()" would be clearer. Since it doesn't change the object, it should be declared const. When you return "const char*", I don't understand why you declare your return variable "char*" and then even need the const_cast to make the compiler happy. You can avoid the return variable entirely using an early return:

const char* 
PartitionListRow::DevicePath() const
{ 
    BBitmapStringField* stringField
        = dynamic_cast<BBitmapStringField*>(GetField(kDeviceColumn));
    
    if (stringField == NULL)
        return NULL; // or return ""; ?

    return stringField->String();
}

Can you provide that as a single patch? Again, thanks a lot for your work!

by jwlh172, 7 years ago

New patch; replaces all above patches; all together in this one.

comment:8 by jwlh172, 7 years ago

I've implemented the latest suggestions from stippi. The newest attached patch includes all modifications.

comment:9 by stippi, 7 years ago

Resolution: fixed
Status: newclosed

Committed in hrev44647. Sorry for the huge delay and thanks again!

Note: See TracTickets for help on using tickets.