From kde-core-devel Fri Oct 17 22:12:57 2014 From: "Kevin Kofler" Date: Fri, 17 Oct 2014 22:12:57 +0000 To: kde-core-devel Subject: Re: Review Request 120627: Remove kdelibs4support. Message-Id: <20141017221257.21223.78349 () probe ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=141358400525315 --===============4097452284455906940== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120627/#review68628 ----------------------------------------------------------- (Review of revision 1, because you posted revision 2 while I was still looking at revision 1.) Please make sure you also test Kompare with remote URLs (e.g. text files served over http://). Both the command-line parsing and the downloading code have issues related to remote URLs that I can see while proofreading and that you have apparently not noticed. Please also test the radio buttons in the dialogs where you replaced KButtonGroup with QGroupBox to ensure that they still behave correctly. komparepart/kompare_part.cpp This line looks very wrong to me: You're executing the block only if the stat job failed? This needs at least the '!' removed. Please make sure you test this snippet, which you changed significantly. komparepart/kompare_part.cpp The filter format is not correct for QFileDialog, it expects this format: "Patch Files (*.diff *.dif *.patch)" libdialogpages/diffpage.cpp QUrl::fromLocalFile is probably the wrong function to call on something called m_excludeFilesFileURL. (See also the discussion of the command line handling in main.cpp.) libdialogpages/diffpage.cpp I think QUrl("diff") should really be good enough here, it's not an absolute path, and we don't need a file://diff URL, we can leave the scheme unset (relative URL). libdialogpages/diffpage.cpp The layout->setSpacing calls can be deleted entirely. KDialog::spacingHint in current kdelibs4 only returned the default layout spacing that QLayout uses anyway. libdialogpages/diffpage.cpp The layout->setMargin calls can be deleted entirely. KDialog::marginHint in current kdelibs4 only returned the default child margin that QLayout uses anyway. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/diffpage.cpp See above. libdialogpages/filespage.cpp See above. libdialogpages/filespage.cpp See above. libdialogpages/filespage.cpp QUrl::fromLocalFile can't be right here. You may need the same treatment as for the command line (see main.cpp). libdialogpages/filespage.cpp See above. libdialogpages/filespage.cpp See above. libdialogpages/filespage.cpp See above. libdialogpages/pagebase.cpp This one should wait until it passes your own testing. main.cpp All these parser.addOption calls must come before the call to parser.process. main.cpp I think this now needs something like: QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0)) QUrl really needs a static method for that, neither the constructor, nor fromLocalFile, nor fromUserInput do the right thing in this context. I'm going to ask on kde-core-devel if I'm missing something. This shows up so often in this function that it might be worth putting it into a helper function. main.cpp Here too: QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0)) main.cpp Here too: QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1)) main.cpp Here too: QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0)) main.cpp Here too: QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1)) main.cpp Here too: QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0)) main.cpp Here too: QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0)) main.cpp Here too: QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1)) - Kevin Kofler On Okt. 17, 2014, 9:38 nachm., Jeremy Whiting wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120627/ > ----------------------------------------------------------- > > (Updated Okt. 17, 2014, 9:38 nachm.) > > > Review request for kdelibs and Kevin Kofler. > > > Repository: kompare > > > Description > ------- > > Change KUrl to QUrl. > Use QLayout/QFrame instead of KVBox (seems broken though somehow) > Use QFileDialog instead of KFileDialog. > > > Diffs > ----- > > libdialogpages/viewpage.cpp 07bdba5e1edf55a6dcd02e5deef58d30c07660c2 > libdialogpages/viewsettings.h dbf6afe0d0c70e548e32dfc09391d67ef595cdba > libdialogpages/viewsettings.cpp 5a69d0bd9a49f7a3881940c4ea8ad407be56adc1 > main.cpp 4132c8442f8546ee7d365051dda0e32196249217 > libdialogpages/diffpage.h 37490b1ebb245e9648530429da63a9240010dddd > libdialogpages/diffpage.cpp 7800b486e023cffe41e1fa3e9e60781250ea4199 > libdialogpages/filespage.h 42afafcd0fc8bc0a01e32b79d414742937d791fb > libdialogpages/filespage.cpp 6a87fe36abd57bdaa09b516de38969db6c6f2298 > libdialogpages/filessettings.h dc3306e34fe1b4eb7cb6a9d2b598f91932bedda0 > libdialogpages/filessettings.cpp 0e19dc00f22a2f6e9588bf2d110dbde682888472 > libdialogpages/pagebase.h 0cef46feaa2cc81deff12c2c5f739e6be6df1b49 > libdialogpages/pagebase.cpp ba1574aed7124ede49e1c5908a8fe693cf7bc5d3 > libdialogpages/viewpage.h b5b770d1441650564106e1cc7ef7e587f6ee142d > komparepart/komparesplitter.cpp 8d496bf279caa7cb9a305c2d15131f591c48818d > kompareurldialog.h dc50c588e70835ad9292da1baf5222f58f512f67 > kompareurldialog.cpp 7de050bc44770a79f8f7d789cabd95d6707a40f1 > libdialogpages/CMakeLists.txt 769a1154c56e8eb8aa42f1bc6d84e0f9a4154fd0 > libdialogpages/dialogpagesexport.h b2de57f6616739d353d4889ef4965ab07f1191aa > komparenavtreepart/komparenavtreepart.cpp 3faceff78fbbd2f083cd0a7837c74f50fe543474 > komparepart/CMakeLists.txt 09b61e6ca0cdce391fc759be49a672a050cc16cd > komparepart/kompare_part.h 24475f1b0ccf7fbeda56860a9a69955cd0b82808 > komparepart/kompare_part.cpp 4d40be0dedcfb91b77ee239de11188b328f8bc13 > komparepart/komparelistview.cpp 35bbab849d8b7938cba518e97a00ed50cae35612 > komparepart/kompareprefdlg.cpp 118485663390e9563a77741b490a9cdf8bf6d464 > komparepart/komparesaveoptionswidget.cpp 4c9acba6a7f9c6dda04130946faac37138422875 > CMakeLists.txt 38167c2099d0ea1600bd5a6893982e809902fa3a > doc/index.docbook 578d12a41d9a6afed441ffd38c39bff16c096ab2 > interfaces/kompareinterface.h 53b19d944b2a4a65c14ea41b8f1c0997581933db > kompare_shell.h 8549fcdc4d1536c58734f2bc3a78b9ebc42c6c5f > kompare_shell.cpp dcc45513f3f9f5f94869046989b6b4f5b1c0995e > komparenavtreepart/CMakeLists.txt 53e8e670e70629afac9197fc108d844733ec5c07 > > Diff: https://git.reviewboard.kde.org/r/120627/diff/ > > > Testing > ------- > > It builds and runs. The compare dialog ui looks squished though and doesn't resize like it used to, must be something I did wrong when porting away from KVBox > > > Thanks, > > Jeremy Whiting > > --===============4097452284455906940== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120627/ 

(Review of revision 1, because you posted revision 2 while I was still looking at revision 1.)


Please make sure you also test Kompare with remote URLs (e.g. text files served over http://). Both the command-line parsing and the downloading code have issues related to remote URLs that I can see while proofreading and that you have apparently not noticed.


Please also test the radio buttons in the dialogs where you replaced KButtonGroup with QGroupBox to ensure that they still behave correctly.

komparepart/kompare_part.cpp (Diff revision 1) 
bool KomparePart::openDiff3( const QString& diff3Output )
290 
		if ( !node.isDir() )
 287 
        if (!statJob->exec() )
 
This line looks very wrong to me: You're executing the block only if the stat job failed? This needs at least the '!' removed.


Please make sure you test this snippet, which you changed significantly.

komparepart/kompare_part.cpp (Diff revision 1) 
bool KomparePart::saveAll()
552 
			KUrl url = KFileDialog::getSaveUrl( m_info.destination.url(),
 555 
			QUrl url = QFileDialog::getSaveFileUrl( widget(), i18n( "Save .diff" ),
 
The filter format is not correct for QFileDialog, it expects this format:
"Patch Files (.diff .dif *.patch)"

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::setSettings( DiffSettings* setts )
99 
	m_excludeFileURLComboBox->setUrl ( KUrl( m_settings->m_excludeFilesFileURL ) );
 94 
	m_excludeFileURLComboBox->setUrl ( QUrl::fromLocalFile( m_settings->m_excludeFilesFileURL ) );
 
QUrl::fromLocalFile is probably the wrong function to call on something called m_excludeFilesFileURL. (See also the discussion of the command line handling in main.cpp.)

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::apply()
146 
	m_diffURLRequester->setUrl( KUrl( "diff" ) );
 141 
	m_diffURLRequester->setUrl( QUrl::fromLocalFile( "diff" ) );
 
I think QUrl("diff") should really be good enough here, it's not an absolute path, and we don't need a file://diff URL, we can leave the scheme unset (relative URL).

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::slotExcludeFileToggled( bool on )
201 
	layout->setSpacing( KDialog::spacingHint() );
 196 
	//layout->setSpacing( KDialog::spacingHint() );
 
The layout->setSpacing calls can be deleted entirely. KDialog::spacingHint in current kdelibs4 only returned the default layout spacing that QLayout uses anyway.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::slotExcludeFileToggled( bool on )
202 
	layout->setMargin( KDialog::marginHint() );
 197 
	//layout->setMargin( KDialog::marginHint() );
 
The layout->setMargin calls can be deleted entirely. KDialog::marginHint in current kdelibs4 only returned the default child margin that QLayout uses anyway.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addDiffTab()
226 
	layout->setSpacing( KDialog::spacingHint() );
 221 
	//layout->setSpacing( KDialog::spacingHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addDiffTab()
227 
	layout->setMargin( KDialog::marginHint() );
 222 
	//layout->setMargin( KDialog::marginHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addFormatTab()
276 
	layout->setSpacing( KDialog::spacingHint() );
 271 
	//layout->setSpacing( KDialog::spacingHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addFormatTab()
277 
	layout->setMargin( KDialog::marginHint() );
 272 
	//layout->setMargin( KDialog::marginHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addOptionsTab()
308 
	groupLayout->setMargin( KDialog::marginHint() );
 303 
	//groupLayout->setMargin( KDialog::marginHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addOptionsTab()
366 
	layout->setSpacing( KDialog::spacingHint() );
 361 
	//layout->setSpacing( KDialog::spacingHint() );
 
See above.

libdialogpages/diffpage.cpp (Diff revision 1) 
void DiffPage::addOptionsTab()
367 
	layout->setMargin( KDialog::marginHint() );
 362 
	//layout->setMargin( KDialog::marginHint() );
 
See above.

libdialogpages/filespage.cpp (Diff revision 1)
41 
	layout->setSpacing( KDialog::spacingHint() );
 37 
	//layout->setSpacing( KDialog::spacingHint() );
 
See above.

libdialogpages/filespage.cpp (Diff revision 1)
42 
	layout->setMargin( KDialog::marginHint() );
 38 
	//layout->setMargin( KDialog::marginHint() );
 
See above.

libdialogpages/filespage.cpp (Diff revision 1) 
void FilesPage::setSecondGroupBoxTitle( const QString& title )
114 
	m_firstURLComboBox->setUrl( KUrl( m_firstURLComboBox->currentText() ) );
 110 
	m_firstURLComboBox->setUrl( QUrl::fromLocalFile( m_firstURLComboBox->currentText() ) );
 
QUrl::fromLocalFile can't be right here. You may need the same treatment as for the command line (see main.cpp).

libdialogpages/filespage.cpp (Diff revision 1) 
void FilesPage::setSecondGroupBoxTitle( const QString& title )
115 
	m_secondURLComboBox->setUrl( KUrl( m_secondURLComboBox->currentText() ) );
 111 
	m_secondURLComboBox->setUrl( QUrl::fromLocalFile( m_secondURLComboBox->currentText() ) );
 
See above.

libdialogpages/filespage.cpp (Diff revision 1) 
void FilesPage::setSecondURLRequesterMode( unsigned int mode )
134 
	m_firstURLComboBox->setUrl( KUrl( m_settings->m_lastChosenSourceURL ) );
 130 
	m_firstURLComboBox->setUrl( QUrl::fromLocalFile( m_settings->m_lastChosenSourceURL ) );
 
See above.

libdialogpages/filespage.cpp (Diff revision 1) 
void FilesPage::setSecondURLRequesterMode( unsigned int mode )
136 
	m_secondURLComboBox->setUrl( KUrl( m_settings->m_lastChosenDestinationURL ) );
 132 
	m_secondURLComboBox->setUrl( QUrl::fromLocalFile( m_settings->m_lastChosenDestinationURL ) );
 
See above.

libdialogpages/pagebase.cpp (Diff revision 1)
24 
PageBase::PageBase() : KVBox()
 24 
PageBase::PageBase() : QFrame()
 
This one should wait until it passes your own testing.

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
81 
	options.add("+-");
 82 
	parser.addOption(QCommandLineOption("c", i18n("This will compare URL1 with URL2")));
 
All these parser.addOption calls must come before the call to parser.process.

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
126 
					ks->openDiff( args->url( 0 ) );
 130 
					ks->openDiff( args.at( 0 ) );
 
I think this now needs something like:
QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0))


QUrl really needs a static method for that, neither the constructor, nor fromLocalFile, nor fromUserInput do the right thing in this context. I'm going to ask on kde-core-devel if I'm missing something.


This shows up so often in this function that it might be worth putting it into a helper function.

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
141 
				KUrl url0 = args->url( 0 );
 145 
				QUrl url0 = QUrl::fromLocalFile(args.at( 0 ));
 
Here too:
QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
143 
				KUrl url1 = args->url( 1 );
 147 
				QUrl url1 = QUrl::fromLocalFile(args.at( 1 ));
 
Here too:
QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
161 
				KUrl url0 = args->url( 0 );
 165 
				QUrl url0 = QUrl::fromLocalFile(args.at( 0 ));
 
Here too:
QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
163 
				KUrl url1 = args->url( 1 );
 167 
				QUrl url1 = QUrl::fromLocalFile(args.at( 1 ));
 
Here too:
QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
177 
				ks->openDiff( args->url( 0 ) );
 181 
				ks->openDiff( QUrl::fromLocalFile(args.at( 0 ) ) );
 
Here too:
QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
187 
			KUrl url0 = args->url( 0 );
 191 
			QUrl url0 = QUrl::fromLocalFile(args.at( 0 ));
 
Here too:
QDir::isAbsolutePath(args.at(0)) ? QUrl::fromLocalFile(args.at(0)) : QUrl(args.at(0))

main.cpp (Diff revision 1) 
static const char version[] = "4.1.3";
189 
			KUrl url1 = args->url( 1 );
 193 
			QUrl url1 = QUrl::fromLocalFile(args.at( 1 ));
 
Here too:
QDir::isAbsolutePath(args.at(1)) ? QUrl::fromLocalFile(args.at(1)) : QUrl(args.at(1))

- Kevin Kofler


On Oktober 17th, 2014, 9:38 nachm. UTC, Jeremy Whiting wrote:

Review request for kdelibs and Kevin Kofler.
By Jeremy Whiting.

Updated Okt. 17, 2014, 9:38 nachm.

Repository: kompare

Description 

Change KUrl to QUrl.
Use QLayout/QFrame instead of KVBox (seems broken though somehow)
Use QFileDialog instead of KFileDialog.

Testing 

It builds and runs. The compare dialog ui looks squished though and doesn't resize like it used to, must be something I did wrong when porting away from KVBox

Diffs

  • libdialogpages/viewpage.cpp (07bdba5e1edf55a6dcd02e5deef58d30c07660c2)
  • libdialogpages/viewsettings.h (dbf6afe0d0c70e548e32dfc09391d67ef595cdba)
  • libdialogpages/viewsettings.cpp (5a69d0bd9a49f7a3881940c4ea8ad407be56adc1)
  • main.cpp (4132c8442f8546ee7d365051dda0e32196249217)
  • libdialogpages/diffpage.h (37490b1ebb245e9648530429da63a9240010dddd)
  • libdialogpages/diffpage.cpp (7800b486e023cffe41e1fa3e9e60781250ea4199)
  • libdialogpages/filespage.h (42afafcd0fc8bc0a01e32b79d414742937d791fb)
  • libdialogpages/filespage.cpp (6a87fe36abd57bdaa09b516de38969db6c6f2298)
  • libdialogpages/filessettings.h (dc3306e34fe1b4eb7cb6a9d2b598f91932bedda0)
  • libdialogpages/filessettings.cpp (0e19dc00f22a2f6e9588bf2d110dbde682888472)
  • libdialogpages/pagebase.h (0cef46feaa2cc81deff12c2c5f739e6be6df1b49)
  • libdialogpages/pagebase.cpp (ba1574aed7124ede49e1c5908a8fe693cf7bc5d3)
  • libdialogpages/viewpage.h (b5b770d1441650564106e1cc7ef7e587f6ee142d)
  • komparepart/komparesplitter.cpp (8d496bf279caa7cb9a305c2d15131f591c48818d)
  • kompareurldialog.h (dc50c588e70835ad9292da1baf5222f58f512f67)
  • kompareurldialog.cpp (7de050bc44770a79f8f7d789cabd95d6707a40f1)
  • libdialogpages/CMakeLists.txt (769a1154c56e8eb8aa42f1bc6d84e0f9a4154fd0)
  • libdialogpages/dialogpagesexport.h (b2de57f6616739d353d4889ef4965ab07f1191aa)
  • komparenavtreepart/komparenavtreepart.cpp (3faceff78fbbd2f083cd0a7837c74f50fe543474)
  • komparepart/CMakeLists.txt (09b61e6ca0cdce391fc759be49a672a050cc16cd)
  • komparepart/kompare_part.h (24475f1b0ccf7fbeda56860a9a69955cd0b82808)
  • komparepart/kompare_part.cpp (4d40be0dedcfb91b77ee239de11188b328f8bc13)
  • komparepart/komparelistview.cpp (35bbab849d8b7938cba518e97a00ed50cae35612)
  • komparepart/kompareprefdlg.cpp (118485663390e9563a77741b490a9cdf8bf6d464)
  • komparepart/komparesaveoptionswidget.cpp (4c9acba6a7f9c6dda04130946faac37138422875)
  • CMakeLists.txt (38167c2099d0ea1600bd5a6893982e809902fa3a)
  • doc/index.docbook (578d12a41d9a6afed441ffd38c39bff16c096ab2)
  • interfaces/kompareinterface.h (53b19d944b2a4a65c14ea41b8f1c0997581933db)
  • kompare_shell.h (8549fcdc4d1536c58734f2bc3a78b9ebc42c6c5f)
  • kompare_shell.cpp (dcc45513f3f9f5f94869046989b6b4f5b1c0995e)
  • komparenavtreepart/CMakeLists.txt (53e8e670e70629afac9197fc108d844733ec5c07)

View Diff
--===============4097452284455906940==--