Zandronum Chat @ irc.zandronum.com
#zandronum
Get the latest version: 3.0
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
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 Informationhttps://www.winrar-france.fr/winrar_instructions_for_use/source/html/HELPIntelCRC.htm [^]
Attached Files

- Relationships
parent of 0003523closedPol M Add support for textboxes in "about" dialogs of plugins 
child of 0003237closedWubTheCaptain Non-free files and copyright issues in source archive 

-  Notes
User avatar (0019757)
Pol M (developer)
2018-09-25 16:05

Should it be appended to the current licence?
User avatar (0019760)
WubTheCaptain (developer)
2018-09-25 18:17

Quote from Pol M
Should it be appended to the current licence?


I believe so, in another comment.
User avatar (0019761)
WubTheCaptain (developer)
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.
User avatar (0019762)
Pol M (developer)
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).

User avatar (0019763)
Zalewa (developer)
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.

User avatar (0019766)
Pol M (developer)
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!๐Ÿ˜‰
User avatar (0019767)
Zalewa (developer)
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.

User avatar (0019771)
WubTheCaptain (developer)
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.
User avatar (0019797)
Pol M (developer)
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)

User avatar (0019799)
WubTheCaptain (developer)
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:").
User avatar (0019809)
Zalewa (developer)
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>


User avatar (0019810)
Pol M (developer)
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.

User avatar (0019811)
Pol M (developer)
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.
User avatar (0019813)
Zalewa (developer)
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.
User avatar (0019816)
Pol M (developer)
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)

User avatar (0019833)
Zalewa (developer)
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.
User avatar (0019841)
Pol M (developer)
2018-09-30 11:29

pull request
User avatar (0019842)
Zalewa (developer)
2018-09-30 11:38

If Linda agrees with your changes, we can merge them and close this ticket as resolved.
User avatar (0019845)
Zalewa (developer)
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...
User avatar (0019847)
Pol M (developer)
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?"๐Ÿ˜‚
User avatar (0019848)
WubTheCaptain (developer)
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.

User avatar (0019849)
WubTheCaptain (developer)
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".

User avatar (0019861)
Pol M (developer)
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)

User avatar (0019863)
Zalewa (developer)
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).

User avatar (0019864)
WubTheCaptain (developer)
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.
User avatar (0019865)
Pol M (developer)
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 [^]

User avatar (0019870)
WubTheCaptain (developer)
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.
User avatar (0019872)
WubTheCaptain (developer)
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. ๐Ÿ˜„
User avatar (0019875)
Pol M (developer)
2018-10-01 18:36

Commited:https://bitbucket.org/Doomseeker/doomseeker/commits/3ca4ea5f9b45

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2018-09-22 23:12 WubTheCaptain New Issue
2018-09-22 23:12 WubTheCaptain Relationship added child of 0003237
2018-09-24 01:18 Blzut3 Target Version => 1.2
2018-09-25 16:05 Pol M Note Added: 0019757
2018-09-25 16:06 Pol M Assigned To => Pol M
2018-09-25 16:06 Pol M Status new => assigned
2018-09-25 18:17 WubTheCaptain Note Added: 0019760
2018-09-25 18:23 WubTheCaptain Note Added: 0019761
2018-09-25 18:33 Pol M Note Added: 0019762
2018-09-25 18:34 Pol M Note Edited: 0019762 View Revisions
2018-09-25 20:47 Zalewa Note Added: 0019763
2018-09-25 20:48 Zalewa Note Edited: 0019763 View Revisions
2018-09-25 21:09 Pol M Note Added: 0019766
2018-09-25 22:03 Zalewa Note Added: 0019767
2018-09-25 22:07 Zalewa Note Edited: 0019767 View Revisions
2018-09-26 00:24 WubTheCaptain Relationship added parent of 0003523
2018-09-26 00:27 WubTheCaptain Note Added: 0019771
2018-09-28 20:06 Pol M Note Added: 0019797
2018-09-28 20:07 Pol M Note Edited: 0019797 View Revisions
2018-09-29 00:09 WubTheCaptain Note Added: 0019799
2018-09-29 09:19 Zalewa Note Added: 0019809
2018-09-29 09:32 Pol M Note Added: 0019810
2018-09-29 09:33 Pol M Note Edited: 0019810 View Revisions
2018-09-29 09:34 Pol M Note Added: 0019811
2018-09-29 09:48 Zalewa Note Edited: 0019809 View Revisions
2018-09-29 09:50 Zalewa Note Added: 0019813
2018-09-29 12:46 Pol M Note Added: 0019816
2018-09-29 13:01 Pol M Note Edited: 0019816 View Revisions
2018-09-29 21:16 Zalewa Note Added: 0019833
2018-09-30 11:29 Pol M Note Added: 0019841
2018-09-30 11:29 Pol M Status assigned => needs review
2018-09-30 11:38 Zalewa Note Added: 0019842
2018-09-30 19:05 Zalewa Note Added: 0019845
2018-09-30 19:38 Pol M Note Added: 0019847
2018-10-01 02:51 WubTheCaptain Note Added: 0019848
2018-10-01 02:51 WubTheCaptain Status needs review => assigned
2018-10-01 02:55 WubTheCaptain Note Edited: 0019848 View Revisions
2018-10-01 02:55 WubTheCaptain Note Edited: 0019848 View Revisions
2018-10-01 03:00 WubTheCaptain Status assigned => feedback
2018-10-01 03:04 WubTheCaptain Note Added: 0019849
2018-10-01 03:04 WubTheCaptain Status feedback => assigned
2018-10-01 03:07 WubTheCaptain Note Edited: 0019849 View Revisions
2018-10-01 03:08 WubTheCaptain Note Edited: 0019849 View Revisions
2018-10-01 03:08 WubTheCaptain Note Edited: 0019849 View Revisions
2018-10-01 15:40 Pol M Note Added: 0019861
2018-10-01 16:08 Pol M Note Edited: 0019861 View Revisions
2018-10-01 17:06 Zalewa Note Added: 0019863
2018-10-01 17:08 Zalewa Note Edited: 0019863 View Revisions
2018-10-01 17:15 WubTheCaptain Note Added: 0019864
2018-10-01 17:18 Pol M Note Added: 0019865
2018-10-01 17:32 Pol M Note Edited: 0019865 View Revisions
2018-10-01 17:43 WubTheCaptain Note Added: 0019870
2018-10-01 17:43 WubTheCaptain Status assigned => needs review
2018-10-01 17:53 WubTheCaptain Note Added: 0019872
2018-10-01 17:55 WubTheCaptain Note View State: 0019872: private
2018-10-01 18:36 Pol M Note Added: 0019875
2018-10-01 18:40 Pol M Status needs review => needs testing
2018-10-02 05:28 WubTheCaptain Status needs testing => resolved
2018-10-02 05:28 WubTheCaptain Fixed in Version => 1.2
2018-10-02 05:28 WubTheCaptain Resolution open => fixed
2018-10-02 05:34 WubTheCaptain Note View State: 0019872: public
2018-10-27 22:54 WubTheCaptain Status resolved => closed






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker