From 357e2d84bcef9119ffe28a4f1a74f8270151b9c9 Mon Sep 17 00:00:00 2001 From: Bart Hakvoort Date: Tue, 11 Jul 2006 18:13:27 +0000 Subject: [PATCH] lots of fixes and improvements (mostly move-related) * lots of fixes and improvements (mostly move-related) --- ChangeLog | 4 + include/GParted_Core.h | 1 + include/Operation.h | 3 +- src/Dialog_Progress.cc | 10 +- src/GParted_Core.cc | 194 ++++++++++++++++++++++++------------- src/OperationCopy.cc | 2 - src/OperationCreate.cc | 2 - src/OperationDelete.cc | 2 - src/OperationFormat.cc | 2 - src/OperationResizeMove.cc | 92 ++++++++++++------ src/Win_GParted.cc | 6 +- 11 files changed, 208 insertions(+), 110 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0320df7a..c207244e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2006-07-11 Bart Hakvoort + + * lots of fixes and improvements (mostly move-related) + 2006-06-20 Bart Hakvoort * src/GParted_Core.cc: some more work on the order of operations when diff --git a/include/GParted_Core.h b/include/GParted_Core.h index 042c95d3..c907534f 100644 --- a/include/GParted_Core.h +++ b/include/GParted_Core.h @@ -38,6 +38,7 @@ public: void set_user_devices( const std::vector & user_devices ) ; void set_devices( std::vector & devices ) ; + bool snap_to_cylinder( Partition & partition ) ; bool apply_operation_to_disk( Operation * operation ); bool set_disklabel( const Glib::ustring & device_path, const Glib::ustring & disklabel ) ; diff --git a/include/Operation.h b/include/Operation.h index 9ec0f101..0bc5cae2 100644 --- a/include/Operation.h +++ b/include/Operation.h @@ -70,6 +70,7 @@ public: virtual ~Operation() {} virtual void apply_to_visual( std::vector & partitions ) = 0 ; + virtual void create_description() = 0 ; //public variables Device device ; @@ -87,8 +88,6 @@ protected: int find_index_extended( const std::vector & partitions ) ; void insert_unallocated( std::vector & partitions, Sector start, Sector end, bool inside_extended ); - virtual void create_description() = 0 ; - int index ; int index_extended ; }; diff --git a/src/Dialog_Progress.cc b/src/Dialog_Progress.cc index 2519048c..67bf6a14 100644 --- a/src/Dialog_Progress.cc +++ b/src/Dialog_Progress.cc @@ -148,6 +148,9 @@ void Dialog_Progress::update_operation_details( const Gtk::TreeRow & treerow, } //check description and update if necessary + //FIXME: it feels quite inefficient to do this for every row, maybe we should add a new status which indicates + //this row may be changed and should be checked...(otoh, if/when we start displaying the output of the command + //in realtime almost every row is going to need this and such a status might become useless...) if ( operation_details .description != treerow[ treeview_operations_columns .operation_description ] ) treerow[ treeview_operations_columns .operation_description ] = operation_details .description ; @@ -160,8 +163,9 @@ void Dialog_Progress::update_operation_details( const Gtk::TreeRow & treerow, else pulse = true ; - //and update the children.. - for ( unsigned int t = 0 ; t < operation_details .sub_details .size() ; t++ ) + //and update the children.. (since sometimes freshly added treerows are not appended yet to the children() list + //we also check if t < treerow .children() .size()) + for ( unsigned int t = 0 ; t < operation_details .sub_details .size() && t < treerow .children() .size() ; t++ ) update_operation_details( treerow .children()[ t ], operation_details .sub_details[ t ] ) ; } @@ -187,7 +191,7 @@ void Dialog_Progress::on_signal_show() running = true ; pthread_create( & pthread, NULL, Dialog_Progress::static_pthread_apply_operation, this ); - int ms = 200 ; + int ms = 200 ; //'refreshrate' of the treeview.. while ( running ) { if ( ms >= 200 ) diff --git a/src/GParted_Core.cc b/src/GParted_Core.cc index cfed8603..f8d51b76 100644 --- a/src/GParted_Core.cc +++ b/src/GParted_Core.cc @@ -147,7 +147,8 @@ void GParted_Core::set_devices( std::vector & devices ) //try to find all available devices ped_device_probe_all(); - + //FIXME: since parted 1.7.1 i detect an unusable /dev/md/0 device (voyager), take a look at this and + //find out how to filter it from the list lp_device = ped_device_get_next( NULL ); while ( lp_device ) { @@ -228,6 +229,12 @@ void GParted_Core::set_devices( std::vector & devices ) fstab_info .clear() ; } +bool GParted_Core::snap_to_cylinder( Partition & partition ) +{ + //FIXME: insert here basicly the functionality of snap_to_boundaries from parted.c + return true ; +} + bool GParted_Core::apply_operation_to_disk( Operation * operation ) { switch ( operation ->type ) @@ -1031,27 +1038,24 @@ bool GParted_Core::move( const Device & device, Partition & partition_new, std::vector & operation_details ) { - //missing. + //FIXME: sometimes (e.g. when moving a 8mib partition) we get a shrink to 0 because somewhere (i guess in the fsclasses) the newsize is calculated as size - cylsize... + //take a look at this and see at which leven this should be solved. if ( partition_new .get_length() > partition_old .get_length() ) { //first do the move Partition temp = partition_new ; temp .sector_end = partition_new .sector_start + partition_old .get_length() ; if ( check_repair( partition_old, operation_details ) && - move_filesystem( partition_old, temp, operation_details ) ) + move_filesystem( partition_old, temp, operation_details ) && + resize_move_partition( partition_old, + temp, + false, + operation_details, + temp .get_length() ) ) { - //now move the partition - if ( check_repair( temp, operation_details ) && - resize_move_partition( partition_old, - temp, - false, - operation_details, - temp .get_length() ) ) - { - //now the partition and the filesystem are moved, we can grow it.. - partition_new .sector_start = temp .sector_start ; - return resize( device, temp, partition_new, operation_details ) ; - } + //now the partition and the filesystem are moved, we can grow it.. + partition_new .sector_start = temp .sector_start ; + return resize( device, temp, partition_new, operation_details ) ; } return false ; @@ -1095,53 +1099,51 @@ bool GParted_Core::move_filesystem( const Partition & partition_old, Partition & partition_new, std::vector & operation_details ) { - operation_details .push_back( OperationDetails( _("move filesystem.") ) ) ; + //FIXME: i think it's best if we check at this level (in each function) if there are real changes. + //if not, then display the suboperation (in this case 'move filesystem') and add a child (italic) with + //something like 'new and old partition have the same positition. skipping move' and return true.. + if ( partition_new .sector_start < partition_old .sector_start ) + operation_details .push_back( OperationDetails( _("move filesystem to the left") ) ) ; + else + operation_details .push_back( OperationDetails( _("move filesystem to the right") ) ) ; bool succes = false ; if ( open_device_and_disk( partition_old .device_path ) ) { //calculate correct geom voor new location (rounded to cylinder) + //we could use the geom set bij snap_to_cylinder, but this one is 100% reliable, while snap_to_cylinder + //exists merely for informative purposes lp_partition = NULL ; lp_partition = ped_disk_get_partition_by_sector( lp_disk, (partition_old .sector_end + partition_old .sector_start) / 2 ) ; - + if ( lp_partition ) { - //calculate a new partition just to get a correct geometry - //(we could use lp_partition as it is, but this feels safer) - lp_partition = ped_partition_new( lp_disk, - lp_partition ->type, - lp_partition ->fs_type, - partition_new .sector_start, - partition_new .sector_end ) ; + PedConstraint *constraint = NULL ; + constraint = ped_constraint_any( lp_device ) ; - if ( lp_partition ) + if ( constraint ) { - PedConstraint *constraint = NULL ; - constraint = ped_constraint_any( lp_device ) ; - - if ( constraint ) + if ( ped_disk_set_partition_geom( lp_disk, + lp_partition, + constraint, + partition_new .sector_start, + partition_new .sector_end ) ) { - if ( ped_disk_set_partition_geom( lp_disk, - lp_partition, - constraint, - partition_new .sector_start, - partition_new .sector_end ) ) - { - partition_new .sector_start = lp_partition ->geom .start ; - partition_new .sector_end = lp_partition ->geom .end ; - succes = true ; - } - - ped_constraint_destroy( constraint ); + partition_new .sector_start = lp_partition ->geom .start ; + partition_new .sector_end = lp_partition ->geom .end ; + succes = true ; } + + ped_constraint_destroy( constraint ); } } //we don't need disk anymore.. close_disk() ; - //FIXME: only move is startsectors are different from each other... + //do the move.. + //FIXME: test all kinds of moving/resizing combi's with different blocksizes Sector blocksize = 32 ;//FIXME: write an algorithm to determine the optimal blocksize Glib::ustring error_message ; @@ -1153,7 +1155,7 @@ bool GParted_Core::move_filesystem( const Partition & partition_old, operation_details .back() .sub_details .push_back( OperationDetails( "", OperationDetails::NONE ) ) ; - if ( partition_new .sector_start < partition_old .sector_start ) + if ( partition_new .sector_start < partition_old .sector_start ) //move to the left { Sector t = 0 ; for ( ; t < partition_old .get_length() - blocksize ; t+=blocksize ) @@ -1199,16 +1201,17 @@ bool GParted_Core::move_filesystem( const Partition & partition_old, } } else //move to the right.. - {//FIXME: why is this so much slower than moving to the left or a simple copy? most likely there's - //an error in the algorithm (also.. there are (fixable)errors in the e2fsck output right - //after the move if we move to the right..) + {//FIXME: moving to the right still appears slower than moving to the left... + //most likely this has something to do with the fact we're reading from right to left, i guess the + //headers of the disk have to move more.. (check this with the parted people) + //since reading from RTL is only needed in case of overlap we could check for this... Sector t = blocksize ; for ( ; t < partition_old .get_length() - blocksize ; t+=blocksize ) { if ( ! copy_block( lp_device, lp_device, - partition_old .sector_start +1 - t, - partition_new .sector_start +1 - t, + partition_old .sector_end - t, + partition_new .sector_end - t, blocksize, error_message ) ) { @@ -1257,7 +1260,7 @@ bool GParted_Core::move_filesystem( const Partition & partition_old, //reset fraction to -1 to make room for a new one (or a pulsebar) operation_details .back() .sub_details .back() .fraction = -1 ; - if( ! succes ) + if ( ! succes ) { if ( ! error_message .empty() ) operation_details .back() .sub_details .push_back( @@ -1267,8 +1270,6 @@ bool GParted_Core::move_filesystem( const Partition & partition_old, operation_details .back() .sub_details .push_back( OperationDetails( "" + ped_error + "", OperationDetails::NONE ) ) ; } - - } close_device_and_disk() ; @@ -1318,7 +1319,7 @@ bool GParted_Core::resize( const Device & device, succes = resize_move_partition( partition_old, partition_new, - ! get_fs( partition_new .filesystem ) .move, + ! get_fs( partition_new .filesystem ) .move,//FIXME: is this still valid? operation_details ) ; //these 3 are always executed, however, if 1 of them fails the whole operation fails @@ -1344,22 +1345,80 @@ bool GParted_Core::resize_move_partition( const Partition & partition_old, std::vector & operation_details, Sector min_size ) { - operation_details .push_back( OperationDetails( _("resize/move partition") ) ) ; - /*FIXME: try to find fitting descriptions for all situations - if ( partition_new .get_length() < partition_old .get_length() ) - operation_details .push_back( OperationDetails( _("shrink partition") ) ) ; - else if ( partition_new .get_length() > partition_old .get_length() ) - operation_details .push_back( OperationDetails( _("grow partition") ) ) ; - else if ( partition_new .sector_start != partition_old .sector_start ) - operation_details .push_back( OperationDetails( _("move partition") ) ) ; - else + //i'm not too happy with this, but i think it is the correct way from a i18n POV + enum Action { - operation_details .push_back( OperationDetails( _("resize/move partition") ) ) ; + NONE = 0, + MOVE_RIGHT = 1, + MOVE_LEFT = 2, + GROW = 3, + SHRINK = 4, + MOVE_RIGHT_GROW = 5, + MOVE_RIGHT_SHRINK = 6, + MOVE_LEFT_GROW = 7, + MOVE_LEFT_SHRINK = 8 + } ; + Action action = NONE ; + + if ( partition_new .get_length() > partition_old .get_length() ) + action = GROW ; + else if ( partition_new .get_length() < partition_old .get_length() ) + action = SHRINK ; + + if ( partition_new .sector_start > partition_old .sector_start && + partition_new .sector_end > partition_old .sector_end ) + action = action == GROW ? MOVE_RIGHT_GROW : action == SHRINK ? MOVE_RIGHT_SHRINK : MOVE_RIGHT ; + else if ( partition_new .sector_start < partition_old .sector_start && + partition_new .sector_end < partition_old .sector_end ) + action = action == GROW ? MOVE_LEFT_GROW : action == SHRINK ? MOVE_LEFT_SHRINK : MOVE_LEFT ; + + Glib::ustring description ; + switch ( action ) + { + case NONE : + description = _("resize/move partition") ; + break ; + case MOVE_RIGHT : + description = _("move partition to the right") ; + break ; + case MOVE_LEFT : + description = _("move partition to the left") ; + break ; + case GROW : + description = _("grow partition from %1 to %2") ; + break ; + case SHRINK : + description = _("shrink partition from %1 to %2") ; + break ; + case MOVE_RIGHT_GROW : + description = _("move partition to the right and grow it from %1 to %2") ; + break ; + case MOVE_RIGHT_SHRINK : + description = _("move partition to the right and shrink it from %1 to %2") ; + break ; + case MOVE_LEFT_GROW : + description = _("move partition to the left and grow it from %1 to %2") ; + break ; + case MOVE_LEFT_SHRINK : + description = _("move partition to the left and shrink it from %1 to %2") ; + break ; + } + + if ( ! description .empty() && action != NONE && action != MOVE_LEFT && action != MOVE_RIGHT ) + description = String::ucompose( description, + Utils::format_size( partition_old .get_length() ), + Utils::format_size( partition_new .get_length() ) ) ; + + operation_details .push_back( OperationDetails( description ) ) ; + + + if ( action == NONE ) operation_details .back() .sub_details .push_back( - "" + - OperationDetails( _("new and old partition have the same size and positition. continuing anyway") ) + - "" ) ; - }*/ + OperationDetails( + Glib::ustring( "" ) + + _("new and old partition have the same size and positition. continuing anyway") + + Glib::ustring( "" ), + OperationDetails::NONE ) ) ; operation_details .back() .sub_details .push_back( OperationDetails( @@ -1370,6 +1429,7 @@ bool GParted_Core::resize_move_partition( const Partition & partition_old, "", OperationDetails::NONE ) ) ; + //finally the actual resize/move bool return_value = false ; PedConstraint *constraint = NULL ; @@ -1467,7 +1527,7 @@ bool GParted_Core::resize_filesystem( const Partition & partition_old, operation_details .push_back( OperationDetails( _("shrink filesystem") ) ) ; else if ( partition_new .get_length() > partition_old .get_length() ) operation_details .push_back( OperationDetails( _("grow filesystem") ) ) ; - else + else//FIXME: incorrect, should say 'resize/move filesystem' and add the same size blabla as submessage.. operation_details .push_back( OperationDetails( _("new and old partition have the same size. continuing anyway") ) ) ; } diff --git a/src/OperationCopy.cc b/src/OperationCopy.cc index 2be7b7aa..98f16875 100644 --- a/src/OperationCopy.cc +++ b/src/OperationCopy.cc @@ -34,8 +34,6 @@ OperationCopy::OperationCopy( const Device & device, this ->partition_copied = partition_copied ; this ->block_size = block_size ; - create_description() ; - this ->partition_new .add_path( String::ucompose( _("copy of %1"), this ->partition_copied .get_path() ), true ) ; } diff --git a/src/OperationCreate.cc b/src/OperationCreate.cc index f7b97743..41c72ffb 100644 --- a/src/OperationCreate.cc +++ b/src/OperationCreate.cc @@ -29,8 +29,6 @@ OperationCreate::OperationCreate( const Device & device, this ->device = device ; this ->partition_original = partition_orig ; this ->partition_new = partition_new ; - - create_description() ; } void OperationCreate::apply_to_visual( std::vector & partitions ) diff --git a/src/OperationDelete.cc b/src/OperationDelete.cc index 6455732f..319ba731 100644 --- a/src/OperationDelete.cc +++ b/src/OperationDelete.cc @@ -26,8 +26,6 @@ OperationDelete::OperationDelete( const Device & device, const Partition & parti this ->device = device ; this ->partition_original = partition_orig ; - - create_description() ; } void OperationDelete::apply_to_visual( std::vector & partitions ) diff --git a/src/OperationFormat.cc b/src/OperationFormat.cc index 273ee06b..a0cc90c3 100644 --- a/src/OperationFormat.cc +++ b/src/OperationFormat.cc @@ -29,8 +29,6 @@ OperationFormat::OperationFormat( const Device & device, this ->device = device ; this ->partition_original = partition_orig ; this ->partition_new = partition_new ; - - create_description() ; } void OperationFormat::apply_to_visual( std::vector & partitions ) diff --git a/src/OperationResizeMove.cc b/src/OperationResizeMove.cc index 7cd8d9af..35fa6f74 100644 --- a/src/OperationResizeMove.cc +++ b/src/OperationResizeMove.cc @@ -28,8 +28,6 @@ OperationResizeMove::OperationResizeMove( const Device & device, this ->device = device ; this ->partition_original = partition_orig ; this ->partition_new = partition_new ; - - create_description() ; } void OperationResizeMove::apply_to_visual( std::vector & partitions ) @@ -44,36 +42,72 @@ void OperationResizeMove::apply_to_visual( std::vector & partitions ) void OperationResizeMove::create_description() { - //FIXME:make messages more informative by specifying shrink/grow instead of resize. - //if startsector has changed we consider it a move - Sector diff = std::abs( partition_new .sector_start - partition_original .sector_start ) ; - if ( diff ) + //i'm not too happy with this, but i think it is the correct way from a i18n POV + enum Action { - if ( diff > 0 ) - description = String::ucompose( _("Move %1 forward by %2"), - partition_new .get_path(), - Utils::format_size( diff ) ) ; - else - description = String::ucompose( _("Move %1 backward by %2"), - partition_new .get_path(), - Utils::format_size( diff ) ) ; - } - - //check if size has changed - diff = std::abs( partition_original .get_length() - partition_new .get_length() ) ; - if ( diff ) + NONE = 0, + MOVE_RIGHT = 1, + MOVE_LEFT = 2, + GROW = 3, + SHRINK = 4, + MOVE_RIGHT_GROW = 5, + MOVE_RIGHT_SHRINK = 6, + MOVE_LEFT_GROW = 7, + MOVE_LEFT_SHRINK = 8 + } ; + Action action = NONE ; + + if ( partition_new .get_length() > partition_original .get_length() ) + action = GROW ; + else if ( partition_new .get_length() < partition_original .get_length() ) + action = SHRINK ; + + if ( partition_new .sector_start > partition_original .sector_start && + partition_new .sector_end > partition_original .sector_end ) + action = action == GROW ? MOVE_RIGHT_GROW : action == SHRINK ? MOVE_RIGHT_SHRINK : MOVE_RIGHT ; + else if ( partition_new .sector_start < partition_original .sector_start && + partition_new .sector_end < partition_original .sector_end ) + action = action == GROW ? MOVE_LEFT_GROW : action == SHRINK ? MOVE_LEFT_SHRINK : MOVE_LEFT ; + + switch ( action ) { - if ( description .empty() ) - description = String::ucompose( _("Resize %1 from %2 to %3"), - partition_new .get_path(), - Utils::format_size( partition_original .get_length() ), - Utils::format_size( partition_new .get_length() ) ) ; - else - description += " " + String::ucompose( _("and Resize %1 from %2 to %3"), - partition_new .get_path(), - Utils::format_size( partition_original .get_length() ), - Utils::format_size( partition_new .get_length() ) ) ; + case NONE : + description = String::ucompose( _("resize/move %1"), partition_original .get_path() ) ; + description += " (" ; + description += _("new and old partition have the same size and positition. continuing anyway") ; + description += ")" ; + break ; + case MOVE_RIGHT : + description = String::ucompose( _("Move %1 to the right"), partition_original .get_path() ) ; + break ; + case MOVE_LEFT : + description = String::ucompose( _("Move %1 to the left"), partition_original .get_path() ) ; + break ; + case GROW : + description = _("Grow %1 from %2 to %3") ; + break ; + case SHRINK : + description = _("Shrink %1 from %2 to %3") ; + break ; + case MOVE_RIGHT_GROW : + description = _("Move %1 to the right and grow it from %2 to %3") ; + break ; + case MOVE_RIGHT_SHRINK : + description = _("Move %1 to the right and shrink it from %2 to %3") ; + break ; + case MOVE_LEFT_GROW : + description = _("Move %1 to the left and grow it from %2 to %3") ; + break ; + case MOVE_LEFT_SHRINK : + description = _("Move %1 to the left and shrink it from %2 to %3") ; + break ; } + + if ( ! description .empty() && action != NONE && action != MOVE_LEFT && action != MOVE_RIGHT ) + description = String::ucompose( description, + partition_original .get_path(), + Utils::format_size( partition_original .get_length() ), + Utils::format_size( partition_new .get_length() ) ) ; } void OperationResizeMove::apply_normal_to_visual( std::vector & partitions ) diff --git a/src/Win_GParted.cc b/src/Win_GParted.cc index c19b33a1..ff949513 100644 --- a/src/Win_GParted.cc +++ b/src/Win_GParted.cc @@ -616,8 +616,11 @@ void Win_GParted::Add_Operation( OperationType operationtype, break; } - if ( operation ) + //FIXME: do this in two separate steps and be more verbose in case of error.. + if ( operation && gparted_core .snap_to_cylinder( operation ->partition_new ) ) { + operation ->create_description() ; + if ( index >= 0 && index < static_cast( operations .size() ) ) operations .insert( operations .begin() + index, operation ) ; else @@ -1292,6 +1295,7 @@ void Win_GParted::activate_paste() //FIXME: in this case there's no window presented to the user, so he cannot choose the blocksize //i guess this means we have to present a window with the choice (maybe the copydialog, with everything //except the blocksize disabled? + //bleh, this will be fixed as soon as the algorith to determine the optimal blocksize is in place Add_Operation( GParted::COPY, partition_new, 32 ) ; } }