Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUI: Port to Qt #7

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@michaelpm54
Copy link
Contributor

michaelpm54 commented May 22, 2018

Updates:

  • Split pthread commit
  • Fixed ax_check_qt5.m4 indentation
  • Added verdigris license and readme
  • Changes to dists/debian/copyright
  • Changes to authors
  • Split c++ standard commit
  • Fixed improper usage of Doxygen comments /** Model functions. */ like this, now looks like // Model functions
  • Removed newline in Doxygen comments
@DrMcCoy
Copy link
Member

DrMcCoy left a comment

Very nice, this is exactly what I was looking for! :D

There's a few minor things:

  • Two commits could be split into two each
  • Comments meant to group methods shouldn't be Doxygen comments (Doxygen just gets confused by them)
  • There's no need to make a Doxygen comment span two multiple lines if there's no further longer explanation
  • I'd like some an acknowledgement of Verdigris in the AUTHORS file and the debian copyright file

I marked those in the invididual commits as well.

If you have any problems wrangling git into splitting commits or otherwise changing the history with an interactive git rebase, please say so and I'll change the commits myself.

Otherwise, I very happy with this PR. :)

@@ -289,6 +280,7 @@ endif()
set(HAVE_PTHREAD ${CMAKE_USE_PTHREADS_INIT})
if(CMAKE_USE_PTHREADS_INIT)
set(PTHREAD_LIBS ${CMAKE_THREAD_LIBS_INIT})
list(APPEND PHAETHON_LIBRARIES ${PTHREAD_LIBS})

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Can you put the pthread-thing into its own commit, before this one? Because that's technically a separate issue.

Qt5Widgets >= $1 \
Qt5Multimedia >= $1 \
Qt5Concurrent >= $1 \
], [$2], [$3])

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Unfortunately, the indentations here will make this look weird when the configure script runs. So best put this in one long line.

noinst_HEADERS += \
verdigris/wobjectdefs.h \
verdigris/wobjectimpl.h \
$(EMPTY)

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Can you also add a README.Phaethon file into the verdigris directory, with the contents

This is the source of Verdigris (<https://github.com/woboq/verdigris>),
based on the git revision .

(and the revision you based it on filled in there).

Also, add a section for it in the AUTHORS file (probably right after UTF-8 CPP):

Verdigris
*********
   Phaethon uses and includes the header-only library Verdigris
   (<https://github.com/woboq/verdigris>) to use Qt5 without moc. Verdigris
   is licensed under version 3 or later of the Lesser GNU General Public
   License. For the full license texts, see LICENSE.LGPLv3 or
   <https://gnu.org/licenses/lgpl-3.0.html>.

And drop in that license file in plaintext.

Likewise, add the two Verdigris files to dists/debian/copyright:

Files: verdigris/wobjectdefs.h
       verdigris/wobjectimpl.h
Copyright: 2016-2018 Woboq GmbH 
License: LGPL-3.0+

And you then need a section

License: LGPL-3.0+
 This package is free software; you can redistribute it and/or
 modify it under the terms of the GNU Lesser General Public
 License as published by the Free Software Foundation; either
 version 3 of the License, or (at your option) any later version.
 .
 This package is distributed in the hope that it will be useful,
 but WITHOUT ANY WARRANTY; without even the implied warranty of
 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 Lesser General Public License for more details.
 .
 On Debian systems, the complete text of the GNU Library General Public License
 can be found in the file "/usr/share/common-licenses/LGPL-3".

at the bottom of the copyright file.

This should all go within this commit, please.


STD=""
if test "x$with_std" = "xyes"; then
AX_CHECK_COMPILER_FLAGS_VAR([C++], [STD], [-std=c++03])
AX_CHECK_COMPILER_FLAGS_VAR([C++], [STD], [-std=c++14])

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Pushing the C++ standard to C++14 should be it own commit, please.

*/
ResourceTreeItem *itemFromIndex(const QModelIndex &index) const;

/** Model functions. */

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Comments to denote a "section" shouldn't be Doxygen comments (i.e. please use /* here instead of /**)

This applies to resourcetreeitem.h as well and further commits as well.

/** Model functions. */

/** Return the index if it exists, else create it.
*/

This comment has been minimized.

@DrMcCoy

DrMcCoy May 22, 2018

Member

Please remove these line breaks inside the Doxygen comments here. There's no need to make the comment span two lines.

This applies to resourcetreeitem.h as well and further commits as well.

@michaelpm54 michaelpm54 force-pushed the michaelpm54:qt branch from 5517bbb to 7789379 May 22, 2018

@DrMcCoy

This comment has been minimized.

Copy link
Member

DrMcCoy commented May 22, 2018

This looks quite good now, yes. I pushed it to the temporary https://github.com/xoreos/phaethon/tree/qttravis branch to see what Travis CI says. :)

How do you want to be credited in the AUTHORS file now? And should I also change your line in the xoreos AUTHORS file?

@michaelpm54

This comment has been minimized.

Copy link
Contributor Author

michaelpm54 commented May 23, 2018

Michael McAssey <michael.mcassey1@gmail.com>

And yes, please do change the one in xoreos.

@DrMcCoy

This comment has been minimized.

Copy link
Member

DrMcCoy commented May 23, 2018

Merged as 2dc7e82...7d5b853, thanks! :D

@DrMcCoy DrMcCoy closed this May 23, 2018

@DrMcCoy

This comment has been minimized.

Copy link
Member

DrMcCoy commented Jun 23, 2018

Hmm, is there a reason for the dependency on QtMultimedia? It doesn't seem to be needed, it seems. I mean, we do have our own sound mixer, powered by OpenAL, after all.

I'm currently exploring potentially creating release packages of Phaethon. For Linux, I'm using a Debian chroot (Debian Stretch, for Phaethon). The thing is, the QtMultimedia in Debian pulls in pulseaudio, which pulls in systemd... which is not something I can distribute.

So I'd need to compile Qt myself in the chroot. I'd rather not, honestly. And if that can be prevented by not requiring QtMultimedia, that would make me happy.

@michaelpm54 michaelpm54 deleted the michaelpm54:qt branch Jun 24, 2018

@michaelpm54

This comment has been minimized.

Copy link
Contributor Author

michaelpm54 commented Jun 24, 2018

I think it's a relic from a previous iteration of phaethon-qt, in which I used it for sound and/or images.
I removed it and everything seems to be working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.