From kde-devel Tue Oct 26 22:39:40 2004 From: David Faure Date: Tue, 26 Oct 2004 22:39:40 +0000 To: kde-devel Subject: Re: Intended behavior of "File Already Exists" dialog box? Message-Id: <200410270039.40993.faure () kde ! org> X-MARC-Message: https://marc.info/?l=kde-devel&m=109883055124658 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--Boundary-00=_sItfBLlsr4ewJjB" --Boundary-00=_sItfBLlsr4ewJjB Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline On Tuesday 26 October 2004 21:01, Steven P. Ulrick wrote: > In the "File Already Exists" dialog box, let's say that I choose to move any > amount of files and I drag them to a particular destination and chose to > "Move" them. If those files already exist, I only ever get the options as > shown in this screenshot: > http://www.faith4miracle.org/FileAlreadyExists-Move.jpg ["skip", "auto skip" and "overwrite all" not offered] I see. This is due to the (new since a few releases) code which tries to do a direct renaming before launching the full "copy + delete" operation. That code has its own "rename dialog" code, and I didn't want to use the "multiple files" and "skip" flags for it.... for a reason that I don't understand anymore (confusing comment). So I added it now. Testing showed that "skip" was broken (it crashed often), so I fixed that too. There are chances I still missed something though, there are so many cases... Care to test the attached patch (for CVS HEAD) ? > After making my choices to either skip or overwrite, when it gets down to the > last file, the dialog reverts to the undesired "Move" behavior: > http://www.faith4miracle.org/FileAlreadyExists-Copy-LastFile.jpg Well for the last file "Skip", "Auto Skip" or "Cancel" would do exactly the same, they would just stop there. So there's no point in offering 3 choices that do the same thing. "Skip" only makes sense if there are more things to do after skipping this file. -- David Faure, faure@kde.org, sponsored by Trolltech to work on KDE, Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org). --Boundary-00=_sItfBLlsr4ewJjB Content-Type: text/x-diff; charset="iso-8859-1"; name="jobfix.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="jobfix.diff" Index: job.cpp =================================================================== RCS file: /home/kde/kdelibs/kio/kio/job.cpp,v retrieving revision 1.417 diff -u -p -r1.417 job.cpp --- job.cpp 25 Oct 2004 17:38:31 -0000 1.417 +++ job.cpp 26 Oct 2004 22:35:32 -0000 @@ -2472,6 +2472,15 @@ void CopyJob::slotEntries(KIO::Job* job, } } +void CopyJob::skipSrc() +{ + m_dest = d->m_globalDest; + destinationState = d->m_globalDestinationState; + ++m_currentStatSrc; + skip( m_currentSrcURL ); + statCurrentSrc(); +} + void CopyJob::statNextSrc() { m_dest = d->m_globalDest; @@ -2643,6 +2652,26 @@ void CopyJob::skip( const KURL & sourceU dirsToRemove.remove( sourceUrl ); } +bool CopyJob::shouldOverwrite( const QString& path ) const +{ + if ( m_bOverwriteAll ) + return true; + QStringList::ConstIterator sit = m_overwriteList.begin(); + for( ; sit != m_overwriteList.end(); ++sit ) + if ( path.startsWith( *sit ) ) + return true; + return false; +} + +bool CopyJob::shouldSkip( const QString& path ) const +{ + QStringList::ConstIterator sit = m_skipList.begin(); + for( ; sit != m_skipList.end(); ++sit ) + if ( path.startsWith( *sit ) ) + return true; + return false; +} + void CopyJob::slotResultCreatingDirs( Job * job ) { // The dir we are trying to create: @@ -2657,19 +2686,14 @@ void CopyJob::slotResultCreatingDirs( Jo KURL oldURL = ((SimpleJob*)job)->url(); // Should we skip automatically ? if ( m_bAutoSkip ) { - // We dont want to copy files in this directory, so we put it on the skip list + // We don't want to copy files in this directory, so we put it on the skip list m_skipList.append( oldURL.path( 1 ) ); skip( oldURL ); dirs.remove( it ); // Move on to next dir } else { // Did the user choose to overwrite already? - bool bOverwrite = m_bOverwriteAll; - QString destFile = (*it).uDest.path(); - QStringList::Iterator sit = m_overwriteList.begin(); - for( ; sit != m_overwriteList.end() && !bOverwrite; sit++ ) - if ( *sit == destFile.left( (*sit).length() ) ) - bOverwrite = true; - if ( bOverwrite ) { // overwrite => just skip + const QString destFile = (*it).uDest.path(); + if ( shouldOverwrite( destFile ) ) { // overwrite => just skip emit copyingDone( this, ( *it ).uSource, ( *it ).uDest, true /* directory */, false /* renamed */ ); dirs.remove( it ); // Move on to next dir } else { @@ -2865,16 +2889,8 @@ void CopyJob::createNextDir() // Is this URL on the skip list or the overwrite list ? while( it != dirs.end() && udir.isEmpty() ) { - QString dir = (*it).uDest.path(); - bool bCreateDir = true; // we'll create it if it's not in any list - - QStringList::Iterator sit = m_skipList.begin(); - for( ; sit != m_skipList.end() && bCreateDir; sit++ ) - // Is dir a subdirectory of *sit ? - if ( *sit == dir.left( (*sit).length() ) ) - bCreateDir = false; // skip this dir - - if ( !bCreateDir ) { + const QString dir = (*it).uDest.path(); + if ( shouldSkip( dir ) ) { dirs.remove( it ); it = dirs.begin(); } else @@ -3140,16 +3156,9 @@ void CopyJob::copyNextFile() // Is this URL on the skip list ? while (it != files.end() && !bCopyFile) { - bCopyFile = true; - QString destFile = (*it).uDest.path(); - - QStringList::Iterator sit = m_skipList.begin(); - for( ; sit != m_skipList.end() && bCopyFile; sit++ ) - // Is destFile in *sit (or a subdirectory of *sit) ? - if ( *sit == destFile.left( (*sit).length() ) ) - bCopyFile = false; // skip this file - - if (!bCopyFile) { + const QString destFile = (*it).uDest.path(); + bCopyFile = !shouldSkip( destFile ); + if ( !bCopyFile ) { files.remove( it ); it = files.begin(); } @@ -3158,19 +3167,13 @@ void CopyJob::copyNextFile() if (bCopyFile) // any file to create, finally ? { // Do we set overwrite ? - bool bOverwrite = m_bOverwriteAll; // yes if overwrite all - QString destFile = (*it).uDest.path(); + bool bOverwrite; + const QString destFile = (*it).uDest.path(); kdDebug(7007) << "copying " << destFile << endl; if ( (*it).uDest == (*it).uSource ) bOverwrite = false; else - { - // or if on the overwrite list - QStringList::Iterator sit = m_overwriteList.begin(); - for( ; sit != m_overwriteList.end() && !bOverwrite; sit++ ) - if ( *sit == destFile.left( (*sit).length() ) ) - bOverwrite = true; - } + bOverwrite = shouldOverwrite( destFile ); m_bCurrentOperationIsLink = false; KIO::Job * newjob = 0L; @@ -3419,6 +3422,160 @@ void CopyJob::slotResultDeletingDirs( Jo deleteNextDir(); } +void CopyJob::slotResultRenaming( Job* job ) +{ + int err = job->error(); + removeSubjob( job, true, false ); // merge metadata + assert ( subjobs.isEmpty() ); + // Determine dest again + KURL dest = m_dest; + if ( destinationState == DEST_IS_DIR && !m_asMethod ) + dest.addPath( m_currentSrcURL.fileName() ); + if ( err ) + { + // Direct renaming didn't work. Try renaming to a temp name, + // this can help e.g. when renaming 'a' to 'A' on a VFAT partition. + // In that case it's the _same_ dir, we don't want to copy+del (data loss!) + if ( m_currentSrcURL.isLocalFile() && m_currentSrcURL.url(-1) != dest.url(-1) && + m_currentSrcURL.url(-1).lower() == dest.url(-1).lower() && + ( err == ERR_FILE_ALREADY_EXIST || err == ERR_DIR_ALREADY_EXIST ) ) + { + kdDebug(7007) << "Couldn't rename directly, dest already exists. Detected special case of lower/uppercase renaming in same dir, try with 2 rename calls" << endl; + QCString _src( QFile::encodeName(m_currentSrcURL.path()) ); + QCString _dest( QFile::encodeName(dest.path()) ); + KTempFile tmpFile( m_currentSrcURL.directory(false) ); + QCString _tmp( QFile::encodeName(tmpFile.name()) ); + kdDebug(7007) << "CopyJob::slotResult KTempFile status:" << tmpFile.status() << " using " << _tmp << " as intermediary" << endl; + tmpFile.unlink(); + if ( ::rename( _src, _tmp ) == 0 ) + { + if ( !QFile::exists( _dest ) && ::rename( _tmp, _dest ) == 0 ) + { + kdDebug(7007) << "Success." << endl; + err = 0; + } + else + { + // Revert back to original name! + if ( ::rename( _tmp, _src ) != 0 ) { + kdError(7007) << "Couldn't rename " << tmpFile.name() << " back to " << _src << " !" << endl; + // Severe error, abort + Job::slotResult( job ); // will set the error and emit result(this) + return; + } + } + } + } + } + if ( err ) + { + // This code is similar to CopyJob::slotResultConflictCopyingFiles + // but here it's about the base src url being moved/renamed + // (*m_currentStatSrc) and its dest (m_dest), not about a single file. + // It also means we already stated the dest, here. + // On the other hand we haven't stated the src yet (we skipped doing it + // to save time, since it's not necessary to rename directly!)... + + Q_ASSERT( m_currentSrcURL == *m_currentStatSrc ); + + // Existing dest? + if ( ( err == ERR_DIR_ALREADY_EXIST || err == ERR_FILE_ALREADY_EXIST ) + && d->m_interactive ) + { + if (m_reportTimer) + m_reportTimer->stop(); + + // Should we skip automatically ? + if ( m_bAutoSkip ) { + // Move on to next file + skipSrc(); + return; + } else if ( m_bOverwriteAll ) { + ; // nothing to do, stat+copy+del will overwrite + } else { + QString newPath; + // Offer overwrite only if the existing thing is a file + // If src==dest, use "overwrite-itself" + RenameDlg_Mode mode = (RenameDlg_Mode) + ( ( err == ERR_DIR_ALREADY_EXIST ? 0 : + ( m_currentSrcURL == dest ) ? M_OVERWRITE_ITSELF : M_OVERWRITE ) ); + + if ( m_currentStatSrc != m_srcList.fromLast() ) // Not last one + mode = (RenameDlg_Mode) ( mode | M_MULTI | M_SKIP ); + else + mode = (RenameDlg_Mode) ( mode | M_SINGLE ); + + // we lack mtime info for both the src (not stated) + // and the dest (stated but this info wasn't stored) + RenameDlg_Result r = Observer::self()->open_RenameDlg( + this, + err == ERR_FILE_ALREADY_EXIST ? i18n("File Already Exists") : i18n("Already Exists as Folder"), + m_currentSrcURL.prettyURL(0, KURL::StripFileProtocol), + dest.prettyURL(0, KURL::StripFileProtocol), + mode, newPath ); + if (m_reportTimer) + m_reportTimer->start(REPORT_TIMEOUT,false); + + switch ( r ) + { + case R_CANCEL: + { + m_error = ERR_USER_CANCELED; + emitResult(); + return; + } + case R_RENAME: + { + // Set m_dest to the chosen destination + // This is only for this src url; the next one will revert to d->m_globalDest + m_dest.setPath( newPath ); + KIO::Job* job = KIO::stat( m_dest, false, 2, false ); + state = STATE_STATING; + destinationState = DEST_NOT_STATED; + addSubjob(job); + return; + } + case R_AUTO_SKIP: + m_bAutoSkip = true; + // fall through + case R_SKIP: + // Move on to next file + skipSrc(); + return; + case R_OVERWRITE_ALL: + m_bOverwriteAll = true; + break; + case R_OVERWRITE: + // Add to overwrite list + // Note that we add dest, not m_dest. + // This ensures that when moving several urls into a dir (m_dest), + // we only overwrite for the current one, not for all. + // When renaming a single file (m_asMethod), it makes no difference. + kdDebug(7007) << "adding to overwrite list: " << dest.path() << endl; + m_overwriteList.append( dest.path() ); + break; + default: + //assert( 0 ); + break; + } + } + } + + kdDebug(7007) << "Couldn't rename, reverting to normal way, starting with stat" << endl; + //kdDebug(7007) << "KIO::stat on " << m_currentSrcURL << endl; + KIO::Job* job = KIO::stat( m_currentSrcURL, true, 2, false ); + state = STATE_STATING; + addSubjob(job); + m_bOnlyRenames = false; + } + else + { + //kdDebug(7007) << "Renaming succeeded, move on" << endl; + emit copyingDone( this, *m_currentStatSrc, dest, true, true ); + statNextSrc(); + } +} + void CopyJob::slotResult( Job *job ) { //kdDebug(7007) << "CopyJob::slotResult() state=" << (int) state << endl; @@ -3433,137 +3590,9 @@ void CopyJob::slotResult( Job *job ) break; case STATE_RENAMING: // We were trying to do a direct renaming, before even stat'ing { - int err = job->error(); - removeSubjob( job, true, false ); // merge metadata - assert ( subjobs.isEmpty() ); - // Determine dest again - KURL dest = m_dest; - if ( destinationState == DEST_IS_DIR && !m_asMethod ) - dest.addPath( m_currentSrcURL.fileName() ); - if ( err ) - { - // Direct renaming didn't work. Try renaming to a temp name, - // this can help e.g. when renaming 'a' to 'A' on a VFAT partition. - // In that case it's the _same_ dir, we don't want to copy+del (data loss!) - if ( m_currentSrcURL.isLocalFile() && m_currentSrcURL.url(-1) != dest.url(-1) && - m_currentSrcURL.url(-1).lower() == dest.url(-1).lower() && - ( err == ERR_FILE_ALREADY_EXIST || err == ERR_DIR_ALREADY_EXIST ) ) - { - kdDebug(7007) << "Couldn't rename directly, dest already exists. Detected special case of lower/uppercase renaming in same dir, try with 2 rename calls" << endl; - QCString _src( QFile::encodeName(m_currentSrcURL.path()) ); - QCString _dest( QFile::encodeName(dest.path()) ); - KTempFile tmpFile( m_currentSrcURL.directory(false) ); - QCString _tmp( QFile::encodeName(tmpFile.name()) ); - kdDebug(7007) << "CopyJob::slotResult KTempFile status:" << tmpFile.status() << " using " << _tmp << " as intermediary" << endl; - tmpFile.unlink(); - if ( ::rename( _src, _tmp ) == 0 ) - { - if ( !QFile::exists( _dest ) && ::rename( _tmp, _dest ) == 0 ) - { - kdDebug(7007) << "Success." << endl; - err = 0; - } - else - { - // Revert back to original name! - if ( ::rename( _tmp, _src ) != 0 ) { - kdError(7007) << "Couldn't rename " << tmpFile.name() << " back to " << _src << " !" << endl; - // Severe error, abort - Job::slotResult( job ); // will set the error and emit result(this) - return; - } - } - } - } - } - if ( err ) - { - // This code is similar to CopyJob::slotResultConflictCopyingFiles - // but here it's about the base src url being moved/renamed - // (*m_currentStatSrc) and its dest (m_dest), not about a single file. - // It also means we already stated the dest, here. - // On the other hand we haven't stated the src yet (we skipped doing it - // to save time, since it's not necessary to rename directly!)... - - Q_ASSERT( m_currentSrcURL == *m_currentStatSrc ); - - // Existing dest? - if ( ( err == ERR_DIR_ALREADY_EXIST || err == ERR_FILE_ALREADY_EXIST ) - && d->m_interactive ) - { - if (m_reportTimer) - m_reportTimer->stop(); - - QString newPath; - // Offer overwrite only if the existing thing is a file - // If src==dest, use "overwrite-itself" - RenameDlg_Mode mode = (RenameDlg_Mode) - ( ( err == ERR_DIR_ALREADY_EXIST ? 0 : - ( m_currentSrcURL == dest ) ? M_OVERWRITE_ITSELF : M_OVERWRITE ) ); - // I won't use M_MULTI or M_SKIP there. It's too ambiguous: - // we are in the middle of the stat phase, so it's hard to know - // if it will apply to the already-stated-dirs that will be copied later, - // and/oor to the to-be-stated src urls that might be in the same case... - mode = (RenameDlg_Mode) ( mode | M_SINGLE ); - // we lack mtime info for both the src (not stated) - // and the dest (stated but this info wasn't stored) - RenameDlg_Result r = Observer::self()->open_RenameDlg( this, - err == ERR_FILE_ALREADY_EXIST ? i18n("File Already Exists") : i18n("Already Exists as Folder"), - m_currentSrcURL.prettyURL(0, KURL::StripFileProtocol), - dest.prettyURL(0, KURL::StripFileProtocol), - mode, newPath ); - if (m_reportTimer) - m_reportTimer->start(REPORT_TIMEOUT,false); - - switch ( r ) - { - case R_CANCEL: - { - m_error = ERR_USER_CANCELED; - emitResult(); - return; - } - case R_RENAME: - { - // Set m_dest to the chosen destination - // This is only for this src url; the next one will revert to d->m_globalDest - m_dest.setPath( newPath ); - KIO::Job* job = KIO::stat( m_dest, false, 2, false ); - state = STATE_STATING; - destinationState = DEST_NOT_STATED; - addSubjob(job); - return; - } - case R_OVERWRITE: - // Add to overwrite list - // Note that we add dest, not m_dest. - // This ensures that when moving several urls into a dir (m_dest), - // we only overwrite for the current one, not for all. - // When renaming a single file (m_asMethod), it makes no difference. - kdDebug(7007) << "adding to overwrite list: " << dest.path() << endl; - m_overwriteList.append( dest.path() ); - break; - default: - //assert( 0 ); - break; - } - } - - kdDebug(7007) << "Couldn't rename, reverting to normal way, starting with stat" << endl; - //kdDebug(7007) << "KIO::stat on " << m_currentSrcURL << endl; - KIO::Job* job = KIO::stat( m_currentSrcURL, true, 2, false ); - state = STATE_STATING; - addSubjob(job); - m_bOnlyRenames = false; - } - else - { - //kdDebug(7007) << "Renaming succeeded, move on" << endl; - emit copyingDone( this, *m_currentStatSrc, dest, true, true ); - statNextSrc(); - } + slotResultRenaming( job ); + break; } - break; case STATE_LISTING: // recursive listing finished //kdDebug(7007) << "totalSize: " << (unsigned int) m_totalSize << " files: " << files.count() << " dirs: " << dirs.count() << endl; // Was there an error ? Index: jobclasses.h =================================================================== RCS file: /home/kde/kdelibs/kio/kio/jobclasses.h,v retrieving revision 1.151 diff -u -p -r1.151 jobclasses.h --- jobclasses.h 19 Oct 2004 19:03:50 -0000 1.151 +++ jobclasses.h 26 Oct 2004 22:35:32 -0000 @@ -1596,8 +1596,12 @@ namespace KIO { void slotResultDeletingDirs( KIO::Job * job ); void deleteNextDir(); void skip( const KURL & sourceURL ); + void slotResultRenaming( KIO::Job * job ); private: void startRenameJob(const KURL &slave_url); + bool shouldOverwrite( const QString& path ) const; + bool shouldSkip( const QString& path ) const; + void skipSrc(); protected slots: void slotStart(); --Boundary-00=_sItfBLlsr4ewJjB Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline >> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe << --Boundary-00=_sItfBLlsr4ewJjB--