lots of fixes and improvements (mostly move-related)

* lots of fixes and improvements (mostly move-related)

ChangeLog

@@ -1,3 +1,7 @@
+2006-07-11 Bart Hakvoort <hakvoort@cvs.gnome.org>
+
+	* lots of fixes and improvements (mostly move-related)
+
 2006-06-20 Bart Hakvoort <hakvoort@cvs.gnome.org>
 
 	* src/GParted_Core.cc: some more work on the order of operations when

include/GParted_Core.h

@@ -38,6 +38,7 @@ public:
 	void set_user_devices( const std::vector<Glib::ustring> & user_devices ) ;
 	void set_devices( std::vector<Device> & 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 ) ;

include/Operation.h

@@ -70,6 +70,7 @@ public:
 	virtual ~Operation() {}
 	
 	virtual void apply_to_visual( std::vector<Partition> & partitions ) = 0 ;
+	virtual void create_description() = 0 ;
 
 	//public variables
 	Device device ;
@@ -87,8 +88,6 @@ protected:
 	int find_index_extended( const std::vector<Partition> & partitions ) ;
 	void insert_unallocated( std::vector<Partition> & partitions, Sector start, Sector end, bool inside_extended );
 	
-	virtual void create_description() = 0 ;
-
 	int index ;
 	int index_extended ;
 };

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..
+	//and update the children.. (since sometimes freshly added treerows are not appended yet to the children() list
-	for ( unsigned int t = 0 ; t < operation_details .sub_details .size() ; t++ )
+	//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 )

src/GParted_Core.cc

@@ -147,7 +147,8 @@ void GParted_Core::set_devices( std::vector<Device> & 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<Device> & 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<OperationDetails> & 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
+			//now the partition and the filesystem are moved, we can grow it..
-			if ( check_repair( temp, operation_details ) &&
+			partition_new .sector_start = temp .sector_start ;
-			     resize_move_partition( partition_old,
+			return resize( device, temp, partition_new, operation_details ) ;
-					       	    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 ) ;
-			}
 		}
 
 		return false ;
@@ -1095,53 +1099,51 @@ bool GParted_Core::move_filesystem( const Partition & partition_old,
 		   		    Partition & partition_new,
 				    std::vector<OperationDetails> & 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
+			PedConstraint *constraint = NULL ;
-			//(we could use lp_partition as it is, but this feels safer)
+			constraint = ped_constraint_any( lp_device ) ;
-			lp_partition = ped_partition_new( lp_disk,
-						    	  lp_partition ->type,
-						    	  lp_partition ->fs_type,
-						    	  partition_new .sector_start,
-						    	  partition_new .sector_end ) ;
 
-			if ( lp_partition )
+			if ( constraint )
 			{
-				PedConstraint *constraint = NULL ;
+				if ( ped_disk_set_partition_geom( lp_disk,
-				constraint = ped_constraint_any( lp_device ) ;
+								  lp_partition,
-
+								  constraint,
-				if ( constraint )
+								  partition_new .sector_start,
+								  partition_new .sector_end ) )
 				{
-					if ( ped_disk_set_partition_geom( lp_disk,
+					partition_new .sector_start = lp_partition ->geom .start ;
-									  lp_partition,
+					partition_new .sector_end = lp_partition ->geom .end ;
-									  constraint,
+					succes = true ;
-									  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 );
 				}
+					
+				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
+			{//FIXME: moving to the right still appears slower than moving to the left...
-			//an error in the algorithm (also.. there are (fixable)errors in the e2fsck output right
+			//most likely this has something to do with the fact we're reading from right to left, i guess the
-			//after the move if we move to the right..)
+			//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_old .sector_end - t,
-						    	   partition_new .sector_start +1 - 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( "<i>" + ped_error + "</i>", 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<OperationDetails> & operation_details,
 					  Sector min_size )
 {
-	operation_details .push_back( OperationDetails( _("resize/move partition") ) ) ;
+	//i'm not too happy with this, but i think it is the correct way from a i18n POV
-	/*FIXME: try to find fitting descriptions for all situations
+	enum Action
-	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
 	{
-		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( 
-			"<i>" + 
+			OperationDetails(
-			OperationDetails( _("new and old partition have the same size and positition. continuing anyway") ) +
+				Glib::ustring( "<i>" ) +
-			"</i>" ) ;
+				_("new and old partition have the same size and positition. continuing anyway") +
-	}*/
+				Glib::ustring( "</i>" ),
+			OperationDetails::NONE ) ) ;
 
 	operation_details .back() .sub_details .push_back( 
 		OperationDetails(
@@ -1370,6 +1429,7 @@ bool GParted_Core::resize_move_partition( const Partition & partition_old,
 			"</i>",
 		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") ) ) ;
 	}

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 ) ;
 }

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<Partition> & partitions ) 

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<Partition> & partitions ) 

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<Partition> & partitions ) 

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<Partition> & partitions ) 
@@ -44,36 +42,72 @@ void OperationResizeMove::apply_to_visual( std::vector<Partition> & partitions )
 
 void OperationResizeMove::create_description() 
 {
-	//FIXME:make messages more informative by specifying shrink/grow instead of resize. 
+	//i'm not too happy with this, but i think it is the correct way from a i18n POV
-	//if startsector has changed we consider it a move
+	enum Action
-	Sector diff = std::abs( partition_new .sector_start - partition_original .sector_start ) ;
-	if ( diff ) 
 	{
-		if ( diff > 0 )
+		NONE			= 0,
-			description = String::ucompose( _("Move %1 forward by %2"), 
+		MOVE_RIGHT	 	= 1,
-						 	partition_new .get_path(),
+		MOVE_LEFT		= 2,
-						 	Utils::format_size( diff ) ) ;
+		GROW 			= 3,
-		else
+		SHRINK			= 4,
-			description = String::ucompose( _("Move %1 backward by %2"),
+		MOVE_RIGHT_GROW		= 5,
-							partition_new .get_path(),
+		MOVE_RIGHT_SHRINK	= 6,
-							Utils::format_size( diff ) ) ;
+		MOVE_LEFT_GROW		= 7,
-	}
+		MOVE_LEFT_SHRINK	= 8
-			
+	} ;
-	//check if size has changed
+	Action action = NONE ;
-	diff = std::abs( partition_original .get_length() - partition_new .get_length() ) ;
+
-	if ( diff )
+	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() ) 
+		case NONE		:
-			description = String::ucompose( _("Resize %1 from %2 to %3"), 
+			description = String::ucompose( _("resize/move %1"), partition_original .get_path() ) ;
-						 	partition_new .get_path(),
+			description += " (" ;
-							Utils::format_size( partition_original .get_length() ),
+			description += _("new and old partition have the same size and positition. continuing anyway") ;
-							Utils::format_size( partition_new .get_length() ) ) ;
+			description += ")" ;
-		else
+			break ;
-			description += " " + String::ucompose( _("and Resize %1 from %2 to %3"),
+		case MOVE_RIGHT		:
-							       partition_new .get_path(),
+			description = String::ucompose( _("Move %1 to the right"), partition_original .get_path() ) ;
-							       Utils::format_size( partition_original .get_length() ),
+			break ;
-							       Utils::format_size( partition_new .get_length() ) ) ;
+		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<Partition> & partitions )

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<int>( 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 ) ;
 	}
 }