[Konsole-devel] [PATCH] Windows port

[Robert Knight]

Hi,

Thank-you for the patch.  I prefer platform specific #if-defs to be
kept to a minimum.  If you are writing code which would compile on all
platforms but should only be executed on some of them, eg:

+#ifndef Q_WS_WIN
   setPtyChannels(KPtyProcess::AllChannels);
+#else
+  setOutputChannelMode(SeparateChannels);
+#endif

Then it would be better to put the platform specific code in a
sub-class and call it via a virtual function.  This is easier to read
and compile-time breakages in code for other platforms are detected
sooner - see the ProcessInfo class implementations for Linux and
Solaris.  Things are slightly more complex here since you want to use
KProcess as the base class on Windows and KPtyProcess on Unix.  That
could be fixed by making Konsole::Pty not inherit directly from
KProcess but have a KProcess as a field internally and provide a
virtual process() accessor.

+#ifndef Q_WS_WIN
 class KDE_EXPORT Profile : public QSharedData
+#else
+class Profile : public QSharedData
+#endif

Why is KDE_EXPORT being removed here on Windows?  If it is required,
the KDE_EXPORT macro should be replaced with something else (eg.
KONSOLE_TEST_EXPORT) which is #defined appropriately depending on the
platform.

> I would prefer to backport the patch too, since it is rather valuable for us.

Generally speaking, only small bug-fixing patches should be
backported.  Plus as you mentioned yourself, there are still some
problems to iron out on Windows.

Regards,
Robert.

2009/1/31 Andrius da Costa Ribas <andriusmao at gmail.com>:
> Thank you very much, Patrick.
>
> I'm really glad :D
>
> I'm going to test it soon.
> (kde-windows mailing list cc'd)
>
> 2009/1/31 Patrick Spendrin <ps_ml at gmx.de>:
>> Hello,
>>
>> After getting many questions about the status of konsole on windows, I just
>> spoke with ossi about how to port konsole to windows.
>> As he pointed out, I replaced KPtyProcess with KProcess - this runs rather
>> well until now as the codebase seems to be rather clean.
>> I still have the following problems though:
>> - some keys are not understood correctly; I am still investigating this and
>> will change the windows.keytab later.
>> - the tests are not compiled. The main problem here is that the relevant
>> classes are not exported from a library but are included directly in either
>> the kpart or in the konsole executable (it might be possible though to make
>> the kpart a library).
>>
>> Now to my questions:
>> Can you please review and comment on the patch?
>> I would prefer to backport the patch too, since it is rather valuable for
>> us.
>>
>> regards,
>> Patrick
>>
>>
>> --
>> web:                 http://windows.kde.org
>> mailing list:        kde-windows at kde.org
>> irc:                 #kde-windows (irc.freenode.net)
>>
>> _______________________________________________
>> konsole-devel mailing list
>> konsole-devel at kde.org
>> https://mail.kde.org/mailman/listinfo/konsole-devel
>>
>>
> _______________________________________________
> konsole-devel mailing list
> konsole-devel at kde.org
> https://mail.kde.org/mailman/listinfo/konsole-devel
>