MantisBT - Doomseeker
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003511Doomseeker[All Projects] Bugpublic2018-09-22 23:122018-10-27 22:54
ReporterWubTheCaptain 
Assigned ToPol M 
PriorityhighSeverityblockReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version1.1 
Target Version1.2Fixed in Version1.2 
Summary0003511: Turok's CRC code: Missing BSD license
DescriptionPer discussions in ticket 0003237, I think a file in Turok engine plugin needs to include a BSD-license to respect Intel's slicing-by-8 CRC32 algorithm copyright.
Steps To Reproducesrc/plugins/turok2ex/crc.cpp
Additional Information'https://www.winrar-france.fr/winrar_instructions_for_use/source/html/HELPIntelCRC.htm [^]'
TagsNo tags attached.
Relationships
parent of 0003523closed Pol M Add support for textboxes in "about" dialogs of plugins 
child of 0003237closed WubTheCaptain Non-free files and copyright issues in source archive 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2018-09-22 23:12WubTheCaptainNew Issue
2018-09-22 23:12WubTheCaptainRelationship addedchild of 0003237
2018-09-24 01:18Blzut3Target Version => 1.2
2018-09-25 16:05Pol MNote Added: 0019757
2018-09-25 16:06Pol MAssigned To => Pol M
2018-09-25 16:06Pol MStatusnew => assigned
2018-09-25 18:17WubTheCaptainNote Added: 0019760
2018-09-25 18:23WubTheCaptainNote Added: 0019761
2018-09-25 18:33Pol MNote Added: 0019762
2018-09-25 18:34Pol MNote Edited: 0019762bug_revision_view_page.php?bugnote_id=19762#r12009
2018-09-25 20:47ZalewaNote Added: 0019763
2018-09-25 20:48ZalewaNote Edited: 0019763bug_revision_view_page.php?bugnote_id=19763#r12011
2018-09-25 21:09Pol MNote Added: 0019766
2018-09-25 22:03ZalewaNote Added: 0019767
2018-09-25 22:07ZalewaNote Edited: 0019767bug_revision_view_page.php?bugnote_id=19767#r12013
2018-09-26 00:24WubTheCaptainRelationship addedparent of 0003523
2018-09-26 00:27WubTheCaptainNote Added: 0019771
2018-09-28 20:06Pol MNote Added: 0019797
2018-09-28 20:07Pol MNote Edited: 0019797bug_revision_view_page.php?bugnote_id=19797#r12040
2018-09-29 00:09WubTheCaptainNote Added: 0019799
2018-09-29 09:19ZalewaNote Added: 0019809
2018-09-29 09:32Pol MNote Added: 0019810
2018-09-29 09:33Pol MNote Edited: 0019810bug_revision_view_page.php?bugnote_id=19810#r12042
2018-09-29 09:34Pol MNote Added: 0019811
2018-09-29 09:48ZalewaNote Edited: 0019809bug_revision_view_page.php?bugnote_id=19809#r12046
2018-09-29 09:50ZalewaNote Added: 0019813
2018-09-29 12:46Pol MNote Added: 0019816
2018-09-29 13:01Pol MNote Edited: 0019816bug_revision_view_page.php?bugnote_id=19816#r12051
2018-09-29 21:16ZalewaNote Added: 0019833
2018-09-30 11:29Pol MNote Added: 0019841
2018-09-30 11:29Pol MStatusassigned => needs review
2018-09-30 11:38ZalewaNote Added: 0019842
2018-09-30 19:05ZalewaNote Added: 0019845
2018-09-30 19:38Pol MNote Added: 0019847
2018-10-01 02:51WubTheCaptainNote Added: 0019848
2018-10-01 02:51WubTheCaptainStatusneeds review => assigned
2018-10-01 02:55WubTheCaptainNote Edited: 0019848bug_revision_view_page.php?bugnote_id=19848#r12057
2018-10-01 02:55WubTheCaptainNote Edited: 0019848bug_revision_view_page.php?bugnote_id=19848#r12058
2018-10-01 03:00WubTheCaptainStatusassigned => feedback
2018-10-01 03:04WubTheCaptainNote Added: 0019849
2018-10-01 03:04WubTheCaptainStatusfeedback => assigned
2018-10-01 03:07WubTheCaptainNote Edited: 0019849bug_revision_view_page.php?bugnote_id=19849#r12060
2018-10-01 03:08WubTheCaptainNote Edited: 0019849bug_revision_view_page.php?bugnote_id=19849#r12061
2018-10-01 03:08WubTheCaptainNote Edited: 0019849bug_revision_view_page.php?bugnote_id=19849#r12062
2018-10-01 15:40Pol MNote Added: 0019861
2018-10-01 16:08Pol MNote Edited: 0019861bug_revision_view_page.php?bugnote_id=19861#r12064
2018-10-01 17:06ZalewaNote Added: 0019863
2018-10-01 17:08ZalewaNote Edited: 0019863bug_revision_view_page.php?bugnote_id=19863#r12066
2018-10-01 17:15WubTheCaptainNote Added: 0019864
2018-10-01 17:18Pol MNote Added: 0019865
2018-10-01 17:32Pol MNote Edited: 0019865bug_revision_view_page.php?bugnote_id=19865#r12070
2018-10-01 17:43WubTheCaptainNote Added: 0019870
2018-10-01 17:43WubTheCaptainStatusassigned => needs review
2018-10-01 17:53WubTheCaptainNote Added: 0019872
2018-10-01 17:55WubTheCaptainNote View State: 0019872: private
2018-10-01 18:36Pol MNote Added: 0019875
2018-10-01 18:40Pol MStatusneeds review => needs testing
2018-10-02 05:28WubTheCaptainStatusneeds testing => resolved
2018-10-02 05:28WubTheCaptainFixed in Version => 1.2
2018-10-02 05:28WubTheCaptainResolutionopen => fixed
2018-10-02 05:34WubTheCaptainNote View State: 0019872: public
2018-10-27 22:54WubTheCaptainStatusresolved => closed

Notes
(0019757)
Pol M   
2018-09-25 16:05   
Should it be appended to the current licence?
(0019760)
WubTheCaptain   
2018-09-25 18:17   
Quote from Pol M
Should it be appended to the current licence?


I believe so, in another comment.
(0019761)
WubTheCaptain   
2018-09-25 18:23   
I believe one also needs to reproduce the copyright notice in the binary distribution somehow. Probably at Help → About → Plugins → Turok 2 Remaster.
(0019762)
Pol M   
2018-09-25 18:33   
(edited on: 2018-09-25 18:34)
Ok, I'll look how to make it in. I'd take the style from Help -> About (the main textbox) with a link to to the page attached to this ticket. Or maybe something fancier like the JSON licence if you want (If you think that this second option is the correct one, please tell me so).
(0019763)
Zalewa   
2018-09-25 20:47   
(edited on: 2018-09-25 20:48)
Pol,

I agree with you that going with the textbox is the correct approach. Add it to the plugins tab in the About dialog and you should be able to populate it with any text the plugin provides.

The text itself, however, is another thing altogether. There's currently no API to provide such text. You could see how the name and version are inserted. The text that appears there is set in the init() call (example: chocolatedoomengineplugin.cpp, look for init() call in the constructor). However, any data that is generated there in this init() is generated only once and also before the translations are loaded and then it is never generated again. This means that if you add an extra "QString aboutText" field to EnginePlugin::Data, it will never get translated.

Solutions to this can be:

1) Around-the-bush and boiler-platey, but doesn't break ABI:

1.1) Create a "class MAIN_EXPORT TextProvider : public QObject { Q_OBJECT; public: virtual QString provide() = 0; }". Place this somewhere in the serverapi in a textprovider.h file.
1.2) In plugins, create [PluginName]AboutProvider : TextProvider classes, override the 'provide()' method and return the tr()'d about text from there.
1.3) In EnginePlugin::Data create a "QSharedPointer<TextProvider> aboutProvider" field.
1.4) In init() in each plugin, assign "new [PluginName]AboutProvider" to the "Data::aboutProvider" field.

2) More straight-to-the-point, but will break ABI. Note that while this is unprofessional, it has also been very common through the course of the development of this program and also all plugins will require update for the next release anyway.

2.1) Add a "virtual QString aboutText();" method to EnginePlugin with default implementation returning an empty "QString()" object.
2.2) Somewhere in Turok 2 Ex plugin create a localized class that inherits from QObject and utilizes Q_OBJECT macro so that tr() can work. You can call this macro "Turok2ExText". This class should work just fine even when declared in the turok2exengineplugin.cpp file.
2.3) In this class add "public static QString about()" method from which you will return the plugin's about text.
2.4) In Turok2ExEnginePlugin override the method "QString aboutText()" method and make it return the value produced by "Turok2ExText::about()".

I'll leave the final choice to you, or maybe you could come up with something better.
(0019766)
Pol M   
2018-09-25 21:09   
Okay,this will require a little bit of time. 😂

While both sound correct to me, the first route sounds more like stuff I've barely touched. That's the one I'll probably gonna try. Tomorrow/in two days I'll look more closely into it.

Quote from Zalewa

or maybe you could come up with something better.

While not impossible, I trust your knowledge over the project.

Thanks for the guidance, my Lord!😉
(0019767)
Zalewa   
2018-09-25 22:03   
(edited on: 2018-09-25 22:07)
Usually the first solutions that one comes up with aren't the most optimal ones and through subsequent iterations you can find something better.

With regards to ABI and why changes to API cannot be simple: the unfortunate problem of C++ is that you not only need to remember that breaking API (the code interface shared between main program and libraries) will result in a "segmentation fault crash" break of your code. The same crashy-crash problem, although much more insidious one, is caused by breaking ABI. To break the ABI one only needs to alter the size of the structure. How can one do that "unknowingly"? For example by adding a virtual method to the base class, which will alter the hidden vtable and change the size of the parent class, breaking all classes that inherit from it. See here:'https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B [^]'

If you inspect the classes that have the MAIN_EXPORT tag you will see that there are some strange POLYMORPHIC macros present there and that the methods are documented as "virtual" or "pure virtual" despite missing the "virtual" keyword. This is polymorphism implemented through method pointers hidden away in Pimpl (Private Implementation, DPtr, DClass) which allows us to add new virtual methods to the MAIN_EXPORT classes without altering the vtable and thus without breaking the ABI. Thanks to this, adding a new method is quite a nuisance, but at least the program doesn't crash if you deploy new version of doomseeker.exe with old plugins.
(0019771)
WubTheCaptain   
2018-09-26 00:27   
Quote from Zalewa
I agree with you that going with the textbox is the correct approach. Add it to the plugins tab in the About dialog and you should be able to populate it with any text the plugin provides.


Created ticket 0003523 for supporting this. Please discuss there.
(0019797)
Pol M   
2018-09-28 20:06   
(edited on: 2018-09-28 20:07)
Since the code to add the "description" of plugins is being reviewed (at the time of writing. please note that for the moment I have not made a pull request) I think that it's about time to ask what should be put in the About menu.

I know near to nothing about licences (after seeing you discuss ticket 0003512), I only know the bare minimum. It would be nice if either someone else with more knowledge wrote down the text that should go into the menu(s), or that someone gave me a little bit (a lot) of guidance of where to start. (I don't even know what licences the plugins use)
(0019799)
WubTheCaptain   
2018-09-29 00:09   
Quote from Pol M
I think that it's about time to ask what should be put in the About menu.


See "Additional Information" for the text of the license that should go in (minus the introduction, only the part after "Text of BSD license is provided below:").
(0019809)
Zalewa   
2018-09-29 09:19   
(edited on: 2018-09-29 09:48)
It's also necessary to take care into not misleading the users that the entirety of the plugin is on BSD, as the plugin is actually on LGPLv2.1+. If we just present the BSD license text into the about box, user may be mislead into thinking that the whole plugin is on BSD.

I propose contents like this:


Turok 2 EX - Doomseeker plugin

This program is distributed under the terms of the LGPL v2.1 or later.

CRC code on BSD License:

<license text goes here>
(0019810)
Pol M   
2018-09-29 09:32   
(edited on: 2018-09-29 09:33)
Quote from Wub

See "Additional Information" for the text of the license that should go in (minus the introduction, only the part after "Text of BSD license is provided below:").

Yeah, I mean that what should be set for the rest of the plugins. I can leave it blank for the other plugins until for the moment if needed.
(0019811)
Pol M   
2018-09-29 09:34   
Quote from Zalewa

Turok 2 EX

This program is distributed under the terms of the LGPL v2.1 or later.

CRC code on BSD License:

<license text goes here>


I'll take this approach.
(0019813)
Zalewa   
2018-09-29 09:50   
Err, make sure to state "Turok 2 EX - Doomseeker plugin" in the first line as per the edited comment 0003511:0019809. We don't want to ambiguously state that the game is on LGPL.
(0019816)
Pol M   
2018-09-29 12:46   
(edited on: 2018-09-29 13:01)
Ok! You also suggested the addition to links. In the case of Turok, should I put the steam page? I could not find a webpage for the game. (or not put a link at all)
(0019833)
Zalewa   
2018-09-29 21:16   
Why not GOG page instead? :)

But no, I couldn't find any reasonable homepage for Turok neither, so sadly there should be none for this game. We shouldn't endorse shops.
(0019841)
Pol M   
2018-09-30 11:29   
pull request
(0019842)
Zalewa   
2018-09-30 11:38   
If Linda agrees with your changes, we can merge them and close this ticket as resolved.
(0019845)
Zalewa   
2018-09-30 19:05   
Quote from Zalewa

If Linda agrees with your changes, we can merge them and close this ticket as resolved.

Dammit, I've been hasty and merged this already. Well, if something is wrong, there's always another commit...
(0019847)
Pol M   
2018-09-30 19:38   
Quote from Zalewa
Dammit, I've been hasty and merged this already.

😂Well, then I should mark this as "Needs testing?"😂
(0019848)
WubTheCaptain   
2018-10-01 02:51   
(edited on: 2018-10-01 02:55)
Trivial changes desired in Turok2AboutProvider::provide().

Quote
"All rights reserved.\n"


"All rights reserved.\n\n"

Quote
- Redistributions of source code ...
- Redistributions in binary form ...


1. Redistributions of source code ...
2. Redistributions in binary form ...


Yes, the URL in OP uses an unordered list. The source code uses unordered list, except asterisk characters (*) instead of ordered list. Not copyright-critical, but a trivial pedantic thing for consistency.

'https://directory.fsf.org/wiki/License:BSD-2-Clause [^]'
'https://spdx.org/licenses/BSD-2-Clause.html [^]'

Quote
"This plugin is distributed under the terms of the LGPL v2.1 or later.\n\n"
"CRC code on BSD License:\n");


These two lines should be two different translations, not one. (Don't merge it into the BSD-2 clause tr(), please. There should be four tr()s in total.)

Quote from src/plugins/turok2ex/crc.cpp
//------------------------------------------------------------------------------
// Copyright (C) 2015 "Samuel Villarreal"
//------------------------------------------------------------------------------


  1. Remove //-- on both top and bottom
  2. Move the remaining "Copyright (C) 2015 "Samuel Villarreal" text above "This library is free software;"
  3. Insert an empty newline between "Copyright (C)" and "This library is free software;"
  4. Have an empty newline between the two copyright notices and licenses.


You should end up with something like this (shorter in this example for brewity):

//------------------------------------------------------------------------------
// crc.cpp
//------------------------------------------------------------------------------
//
// Copyright (C) 2015 "Samuel Villarreal"
//
// This library is free software; ...
//
// This library is distributed in the hope that it will be useful,
// ...
//
// You should have received a copy ...

// Copyright (c) 2004-2006 Intel Corporation.
// All rights reserved.
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are met:
//
// ...


You may add //-- between the two licenses if you think it improves readability. (I think it's excess and worse for accessibility, especially for screen reader software.)

Quote
Copyright (C) 2018 "Pol Marcet Sardà" <polmarcetsarda@gmail.com>


For future reference, quotes around your name are unnecessary unless it's a pseudonym. I'm aware the Turok 2 plugin uses them for Samuel's name, however rest of the code and common practice in software development does not.
(0019849)
WubTheCaptain   
2018-10-01 03:04   
(edited on: 2018-10-01 03:08)
Quote
Copyright (C) 2018 "Pol Marcet Sardà" <polmarcetsarda@gmail.com>


Also since we're dealing with a big new contribution here, it'd be best for you to disclaim any work-for-hire your potential employer may do on your code. In other words, if you work in software development and have a contract with your employer saying the employer gets assigned copyright on all your software development, ask them to sign off your code.

See appendix of the LGPLv2.1 license, section "How to Apply These Terms to Your New Libraries".
(0019861)
Pol M   
2018-10-01 15:40   
(edited on: 2018-10-01 16:08)
Quote from Linda

In other words, if you work in software development and have a contract with your employer saying the employer gets assigned copyright on all your software development, ask them to sign off your code.


Okay, if I'm understanding this correctly, in case that I work for a company I should add something like this:

Quote from LGPLv2.1

You should also get your employer (if you work as a programmer) or your school, if any, to sign a "copyright disclaimer" for the library, if necessary. Here is a sample; alter the names:

Yoyodyne, Inc., hereby disclaims all copyright interest in
the library `Frob' (a library for tweaking knobs) written
by James Random Hacker.

signature of Ty Coon, 1 April 1990
Ty Coon, President of Vice


The problem is that I do not work for anybody. I'm studying at an institute (not university/college) (they literally have no relation with any of this since it has been written in my free time and they have no classes in computer engineering.) If you insisted that this is necessary, I can ask, but I can guarantee you that they are not interested.

(I haven't failed an exam in my life, I'm just young)
(0019863)
Zalewa   
2018-10-01 17:06   
(edited on: 2018-10-01 17:08)
I risk derailing this ticket a bit, but I'd like to mention that you should be wary of such contracts (SIGN THIS CIROGRAPH AND ALL YOU DO BECOMES OURS) when you seek any job in the future. Your employer should not be interested with software you develop in your own free time, as long as you don't infringe on their rights (like using company's code) or replicate the software, though with that last one it can be rather vague what exactly constitutes "a replica" or "a software in the same realm". I don't know how the matter of such contract would hold up in court, but there are enough jobs in IT that you can safely stay away from contracts like this. If you get a chance to get a job as a student when nearing the end of your course, then take it - there's a high chance that they'll pay you, they'll allow you to prioritize finishing the studies over the job, they'll keep you after you get your degree and you'll skip job interview (I was that lucky 8 years ago).
(0019864)
WubTheCaptain   
2018-10-01 17:15   
Pol M: Just a note to us would've been fine. Since you seem not have an external relationship, I have no more concern with the subject.

I'll await changes proposed.
(0019865)
Pol M   
2018-10-01 17:18   
(edited on: 2018-10-01 17:32)
(Thank you for all your wise and kind words)
Quote from Zalewa

you should be wary of such contracts (SIGN THIS CIROGRAPH AND ALL YOU DO BECOMES OURS) when you seek any job in the future.

Do not worry, I will.

That clarified, I'll make a pull request with the suggestions, except the last one. Wich I do not think applies to my case.

EDIT:'https://bitbucket.org/Doomseeker/doomseeker/pull-requests/31/adress-suggestions-related-to-ticket-3511/diff [^]'
(0019870)
WubTheCaptain   
2018-10-01 17:43   
Quote from Pol M
https://bitbucket.org/Doomseeker/doomseeker/pull-requests/31/adress-suggestions-related-to-ticket-3511/diff


I don't care to fix it, but lines 54–55 would've fit on one line now. Looks good to me, thanks!

Zalewa, please merge.
(0019872)
WubTheCaptain   
2018-10-01 17:53   
Quote from Pol M
It was done intentionally to be easier to read.


Yeah, I can agree on that. Hence I did not bother requesting changes. 😄
(0019875)
Pol M   
2018-10-01 18:36   
Commited:https://bitbucket.org/Doomseeker/doomseeker/commits/3ca4ea5f9b45