> On Aug. 12, 2013, 10:53 p.m., Patrick Spendrin wrote: > > kioslave/thumbnail/windowsexecreator.h, line 30 > > <http://git.reviewboard.kde.org/r/112009/diff/2/?file=178073#file178073line30> > > > > style issue: check that you keep the eol at the end of the file (your > > patch seems to have "no eol at the end of file" in it)
It actually had 2 linefeeds at end, and I changed to one. > On Aug. 12, 2013, 10:53 p.m., Patrick Spendrin wrote: > > kioslave/thumbnail/CMakeLists.txt, line 160 > > <http://git.reviewboard.kde.org/r/112009/diff/2/?file=178067#file178067line160> > > > > this could be moved out of the parentheses right? > > Andrius da Costa Ribas wrote: > I didn't get this one, what can be moved out? > > Patrick Spendrin wrote: > This line seems to be in both parts of the if(WIN32) clause, so either it > is wrong? or you can move it out of the parentheses? > set ( windowsimagethumbnail_SRCS ${windowsimagethumbnail_SRCS} > icoutils_win.cpp ) oops, shouldn't be _win on the "not win32" side. Done! > On Aug. 12, 2013, 10:53 p.m., Patrick Spendrin wrote: > > kioslave/thumbnail/icoutils_win.cpp, line 29 > > <http://git.reviewboard.kde.org/r/112009/diff/2/?file=178072#file178072line29> > > > > I don't think we should use dynamic loading of those functions here, > > only for those functions that might not be available in one of the Windows > > versions we support (XPsp3, Vista, 7, 8). For most of these functions you > > can simply use the linked version (e.g. directly use LoadLibraryEx instead > > of loading kernel32, finding the function, using that function etc.) That > > might save you quite some code. In case one of the functions is not > > available, you might need a workaround or a proper error handling. removed all the dynamic loading. > On Aug. 12, 2013, 10:53 p.m., Patrick Spendrin wrote: > > kioslave/thumbnail/windowsexecreator.cpp, line 49 > > <http://git.reviewboard.kde.org/r/112009/diff/2/?file=178074#file178074line49> > > > > You might simply return the output of !IcoUtils::loadIcoImageFromExe > > here ;-) done! - Andrius da Costa ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112009/#review37623 ----------------------------------------------------------- On Aug. 17, 2013, 1:43 p.m., Andrius da Costa Ribas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112009/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2013, 1:43 p.m.) > > > Review request for KDE Runtime, kdewin and Pali Rohár. > > > Description > ------- > > This patch intends to enable Windows exe/dll thumbnailing by using winapi. It > derives from the unsubmitted patch from Pali Rohár from > https://svn.reviewboard.kde.org/r/5156/ as a starting point. I've made a few > adjustments on the original patch, split that patch into a common part and a > icoutils-specific part, and then created the winapi-based part to replace the > icoutils one on Windows (porting icoutils to windows wasn't going to be easy). > > > Diffs > ----- > > kioslave/thumbnail/CMakeLists.txt b81339b > kioslave/thumbnail/icoutils.h 6468bc1 > kioslave/thumbnail/icoutils.cpp 31db85d > kioslave/thumbnail/icoutils_common.cpp PRE-CREATION > kioslave/thumbnail/icoutils_win.cpp PRE-CREATION > kioslave/thumbnail/icoutils_wrestool.cpp PRE-CREATION > kioslave/thumbnail/windowsexecreator.h a407982 > kioslave/thumbnail/windowsexecreator.cpp 9e24aee > kioslave/thumbnail/windowsexethumbnail.desktop f10efef > kioslave/thumbnail/windowsimagecreator.h 0b68cc6 > kioslave/thumbnail/windowsimagecreator.cpp 08b063d > > Diff: http://git.reviewboard.kde.org/r/112009/diff/ > > > Testing > ------- > > Tested on a Windows 7 64-bit machine, with intel compiler (32-bit). > Tested using ico files and both 32-bit and 64-bit executables and dlls, > including jumbo-size icons. > I've used QLibrary for all winapi functions in order to avoid issues with > MinGW compiler, but I don't have a MinGW setup to check. > > Not tested on *nix, but the original patch was not changed except for > iterating order in the common part and namespacing. > > > File Attachments > ---------------- > > screenshot > http://git.reviewboard.kde.org/media/uploaded/files/2013/08/11/Icons.png > > > Thanks, > > Andrius da Costa Ribas > >
