Added TDE license info dialog #318

Merged
MicheleC merged 1 commits from feat/tde-license-info into master 1 year ago
Owner

As per title. The dialog is shown once per minor TDE release.

On deb distros, to be built with TDE/tde-packaging#212

As per title. The dialog is shown once per minor TDE release. On deb distros, to be built with TDE/tde-packaging#212
MicheleC added this to the R14.1.0 release milestone 1 year ago
MicheleC requested review from SlavekB 1 year ago
MicheleC force-pushed feat/tde-license-info from 8434760d67 to 7321c59117 1 year ago
blu.256 commented 1 year ago
Collaborator

I haven't tested this but I took a quick glance at the code. Here's some notes:

  1. Wouldn't it be better to store licence texts in files? Moreover,all four licences are already distributed with TDE (because they are used by TDEAboutData, which reads the needed licence file like this). Their actual location is under $TDEPREFIX/share/apps/LICENSES/. Approximate code:
TQString TDELicenseDlg::readLicenseFile(TQString licenseName)
{
    TQString licensePath = locate("data", TQString("LICENSES/%1").arg(licenseName));
    if (licensePath.isEmpty())
    {
        return i18n("License file not found!");
    }

    TQFile licenseFile(licensePath);
    if (licenseFile.open(IO_ReadOnly))
    {
        TQTextStream str(&file);
        return str.read();
    }
    
    return i18n("Unable to open license file!");
}

void TDELicenseDlg::slotGPL2()
{
    m_teLicense->setText(readLicenseFile("GPL_V2"));
    m_teLicense->moveCursor(TQTextEdit::MoveHome, false);
}

...
  1. Why use a KTextEdit when a KTextBrowser would be more appropriate?
  2. Some of the code I've encountered in TDE is licensed under BSD-style licenses, but the dialog does not seem to mention them (applications code, I think. I'm trying to find out). Is that right? We might need to specify that the licenses cover the base packages of TDE.
  3. Wouldn't it be a good idea to add a button to the "About Trinity" dialog that would invoke this license dialog?
I haven't tested this but I took a quick glance at the code. Here's some notes: 1) Wouldn't it be better to store licence texts in files? Moreover,all four licences are already distributed with TDE (because they are used by TDEAboutData, which reads the needed licence file [like this](http://trinitydesktop.org/docs/trinity/tdelibs/tdecore/html/tdeaboutdata_8cpp_source.html#l00421)). Their actual location is under `$TDEPREFIX/share/apps/LICENSES/`. Approximate code: ``` TQString TDELicenseDlg::readLicenseFile(TQString licenseName) { TQString licensePath = locate("data", TQString("LICENSES/%1").arg(licenseName)); if (licensePath.isEmpty()) { return i18n("License file not found!"); } TQFile licenseFile(licensePath); if (licenseFile.open(IO_ReadOnly)) { TQTextStream str(&file); return str.read(); } return i18n("Unable to open license file!"); } void TDELicenseDlg::slotGPL2() { m_teLicense->setText(readLicenseFile("GPL_V2")); m_teLicense->moveCursor(TQTextEdit::MoveHome, false); } ... ``` 2) Why use a KTextEdit when a KTextBrowser would be more appropriate? 3) Some of the code I've encountered in TDE is licensed under BSD-style licenses, but the dialog does not seem to mention them (applications code, I think. I'm trying to find out). Is that right? We might need to specify that the licenses cover the base packages of TDE. 4) Wouldn't it be a good idea to add a button to the "About Trinity" dialog that would invoke this license dialog?
blu.256 commented 1 year ago
Collaborator

I've taken a look through tdebase and created this 'little' table of the licenses for each component:

https://wiki.trinitydesktop.org/User:Blu256/Draft:Licenses

The licenses were determined by:
a) Looking at TDEAboutData
b) Looking into COPYING and LICENSE* files
c) Looking at the source code files' header comments

I don't know much about licensing, but I've found that apart from the four licenses you have provided in TDELicenseDlg, there's also the MIT license, the 2-clause BSD license and the Artistic license (and also GNU LGPL version 2.1). Some of these only seem to apply to specific source files, while others for whole components (e.g. the Artistic license seems to apply to TDESU, KDCOP, tdepasswd and kstart).

What do you think about this?

I've taken a look through `tdebase` and created this 'little' table of the licenses for each component: https://wiki.trinitydesktop.org/User:Blu256/Draft:Licenses The licenses were determined by: a) Looking at TDEAboutData b) Looking into COPYING and LICENSE* files c) Looking at the source code files' header comments I don't know much about licensing, but I've found that apart from the four licenses you have provided in TDELicenseDlg, there's also the MIT license, the 2-clause BSD license and the Artistic license (and also GNU LGPL version 2.1). Some of these only seem to apply to specific source files, while others for whole components (e.g. the Artistic license seems to apply to TDESU, KDCOP, tdepasswd and kstart). What do you think about this?
SlavekB commented 1 year ago
Owner

I haven't tested this but I took a quick glance at the code. Here's some notes:

  1. Wouldn't it be better to store licence texts in files? Moreover,all four licences are already distributed with TDE (because they are used by TDEAboutData, which reads the needed licence file like this). Their actual location is under $TDEPREFIX/share/apps/LICENSES/.

Yes, definitely, licenses texts are already part of the installation, so it makes sense to use these existing files instead of the embedding of texts inside the binary.

> I haven't tested this but I took a quick glance at the code. Here's some notes: > 1) Wouldn't it be better to store licence texts in files? Moreover,all four licences are already distributed with TDE (because they are used by TDEAboutData, which reads the needed licence file [like this](http://trinitydesktop.org/docs/trinity/tdelibs/tdecore/html/tdeaboutdata_8cpp_source.html#l00421)). Their actual location is under `$TDEPREFIX/share/apps/LICENSES/`. Yes, definitely, licenses texts are already part of the installation, so it makes sense to use these existing files instead of the embedding of texts inside the binary.
Poster
Owner

Hi Philippe,
thanks for the good feedback.

  1. Wouldn't it be better to store licence texts in files?

Indeed it is a better idea, I didn't realize we were already distributing the files with TDE (although I should have, I guess!).

  1. Why use a KTextEdit when a KTextBrowser would be more appropriate?

I didn't know/remember there is such class. I will test and see how it looks and if it looks good, we can use that.

  1. Some of the code I've encountered in TDE is licensed under BSD-style licenses, but the dialog does not seem to mention them (applications code, I think. I'm trying to find out). Is that right? We might need to specify that the licenses cover the base packages of TDE.

As per point 1., didn't realize TDE is using more licenses. We definitely need to add the missing ones, so I will amend the code as needed.

  1. Wouldn't it be a good idea to add a button to the "About Trinity" dialog that would invoke this license dialog?

Sounds like a good idea. Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog.
What do you think?

Hi Philippe, thanks for the good feedback. > 1) Wouldn't it be better to store licence texts in files? Indeed it is a better idea, I didn't realize we were already distributing the files with TDE (although I should have, I guess!). > 2) Why use a KTextEdit when a KTextBrowser would be more appropriate? I didn't know/remember there is such class. I will test and see how it looks and if it looks good, we can use that. > 3) Some of the code I've encountered in TDE is licensed under BSD-style licenses, but the dialog does not seem to mention them (applications code, I think. I'm trying to find out). Is that right? We might need to specify that the licenses cover the base packages of TDE. As per point 1., didn't realize TDE is using more licenses. We definitely need to add the missing ones, so I will amend the code as needed. > 4) Wouldn't it be a good idea to add a button to the "About Trinity" dialog that would invoke this license dialog? Sounds like a good idea. Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog. What do you think?
MicheleC force-pushed feat/tde-license-info from 7321c59117 to df28191774 1 year ago
Poster
Owner

PR updated based on the feedback from Philippe. I have added the remaining 3 licenses that were part of TDEAboutData but were not included in the original PR. The license text is read from file (thanks for the code :-) ) and I replaced the KTextEdit (which was supposed to be readonly but I had forgotten one line of code) with a KTextBrowser.

Point 4. has not been implemented yet.

Just discussed with Slavek that it will be better to add the MIT license too as pointed out by Philippe, so more changes to come (here and in tdelibs).

PR updated based on the feedback from Philippe. I have added the remaining 3 licenses that were part of TDEAboutData but were not included in the original PR. The license text is read from file (thanks for the code :-) ) and I replaced the KTextEdit (which was supposed to be readonly but I had forgotten one line of code) with a KTextBrowser. Point 4. has not been implemented yet. Just discussed with Slavek that it will be better to add the MIT license too as pointed out by Philippe, so more changes to come (here and in tdelibs).
MicheleC force-pushed feat/tde-license-info from df28191774 to ae275b8f7b 1 year ago
Poster
Owner

MIT license also added. This requires TDE/tdelibs#197.

The dialog looks like this:

image

MIT license also added. This requires TDE/tdelibs#197. The dialog looks like this: ![image](/attachments/0231d7b6-2ca2-4902-9d05-f5398a7a8f1c)
blu.256 commented 1 year ago
Collaborator

Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog.

Both approarches sound good.

PR updated based on the feedback from Philippe. I have added the remaining 3 licenses that were part of TDEAboutData but were not included in the original PR. The license text is read from file (thanks for the code :-) ) and I replaced the KTextEdit (which was supposed to be readonly but I had forgotten one line of code) with a KTextBrowser.

Good work! I might test it later today.

Just discussed with Slavek that it will be better to add the MIT license too as pointed out by Philippe, so more changes to come (here and in tdelibs).

Probably should be added to TDEAboutData class too (just saying...).

> Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog. Both approarches sound good. > PR updated based on the feedback from Philippe. I have added the remaining 3 licenses that were part of TDEAboutData but were not included in the original PR. The license text is read from file (thanks for the code :-) ) and I replaced the KTextEdit (which was supposed to be readonly but I had forgotten one line of code) with a KTextBrowser. Good work! I might test it later today. > Just discussed with Slavek that it will be better to add the MIT license too as pointed out by Philippe, so more changes to come (here and in tdelibs). Probably should be added to TDEAboutData class too (just saying...).
Poster
Owner

Probably should be added to TDEAboutData class too (just saying...).

done, see TDE/tdelibs#197

> Probably should be added to TDEAboutData class too (just saying...). done, see TDE/tdelibs#197
blu.256 commented 1 year ago
Collaborator

The dialog looks like this:

Looks a little unusual to me from a UI point of view. Maybe we could use tabs and preload the licenses into them? This would look more "usual" and would avoid reloading each license when you select another license (thus supposedly making the UI snappier when it has loaded).

> The dialog looks like this: Looks a little unusual to me from a UI point of view. Maybe we could use tabs and preload the licenses into them? This would look more "usual" and would avoid reloading each license when you select another license (thus supposedly making the UI snappier when it has loaded).
SlavekB commented 1 year ago
Owner

For point 4. it seems to be a good idea to add a License tab, because in this way it is also in About dialog for applications. The question is whether to insert as much content there as it is in the newly added separate window, as this could cause a significant window enlargement.

For point 4. it seems to be a good idea to add a License tab, because in this way it is also in About dialog for applications. The question is whether to insert as much content there as it is in the newly added separate window, as this could cause a significant window enlargement.
Poster
Owner

Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog.
Both approarches sound good.

After further tihnking, adding a tab is probably the better of the two. This will require moving most of the code from tdebase into tdelibs (like ktips does) and then loading the contents of the dialog in the related container widget (either the new tab of the tde_license_info dialog).
Since R14.1.0 is going to be soft frozen tonight, I suggest we leave point 4. for R14.1.1. @blu.256 @SlavekB what do you think?

>> Maybe even better, we could add a tab "Licenses" that contains the same contents of the tde_licence_info dialog. > Both approarches sound good. After further tihnking, adding a tab is probably the better of the two. This will require moving most of the code from tdebase into tdelibs (like ktips does) and then loading the contents of the dialog in the related container widget (either the new tab of the tde_license_info dialog). Since R14.1.0 is going to be soft frozen tonight, I suggest we leave point 4. for R14.1.1. @blu.256 @SlavekB what do you think?
blu.256 commented 1 year ago
Collaborator

I suggest we leave point 4. for R14.1.1. @blu.256 @SlavekB what do you think?

Seems reasonable. You can merge the dialog in its current form for R14.1.0 and then come back to improve it in R14.1.1.

> I suggest we leave point 4. for R14.1.1. @blu.256 @SlavekB what do you think? Seems reasonable. You can merge the dialog in its current form for R14.1.0 and then come back to improve it in R14.1.1.
Poster
Owner

would avoid reloading each license when you select another license

I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable.

> would avoid reloading each license when you select another license I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable.
blu.256 commented 1 year ago
Collaborator

I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable.

I agree, but my main point was using a KTabWidget instead of buttons. Pre-loading was just a side-effect that would stem from it.

> I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable. I agree, but my main point was using a KTabWidget instead of buttons. Pre-loading was just a side-effect that would stem from it.
SlavekB commented 1 year ago
Owner

I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable.

I agree, but my main point was using a KTabWidget instead of buttons. Pre-loading was just a side-effect that would stem from it.

Using KTabWidget also seems to me like a good idea.

> > I thought about doing pre-loading, but in the end you would load all files regardless of whether the user clicks on the buttons. Most likely, users won't read those licenses IMO, so lazy loading on demand seems more suitable. > > I agree, but my main point was using a KTabWidget instead of buttons. Pre-loading was just a side-effect that would stem from it. Using KTabWidget also seems to me like a good idea.
MicheleC force-pushed feat/tde-license-info from ae275b8f7b to 0308f2af79 1 year ago
Poster
Owner

Updated the code to use KTabWidget. It now looks like this:
image

Strangely, if I use & in the tab label, it gets shown as such rather than being used as shortcut for that tab :-(

Updated the code to use KTabWidget. It now looks like this: ![image](/attachments/292d663e-8d08-4c97-a4d6-514e83653c51) Strangely, if I use `&` in the tab label, it gets shown as such rather than being used as shortcut for that tab :-(
MicheleC force-pushed feat/tde-license-info from 0308f2af79 to 6c423143f8 1 year ago
SlavekB commented 1 year ago
Owner

Updated dialog box with KTabWidget seems good to me. BTW, we should add CMakeL10n rules to generate a new TDELicense.pot catalog.

Updated dialog box with KTabWidget seems good to me. BTW, we should add CMakeL10n rules to generate a new `TDELicense.pot` catalog.
MicheleC force-pushed feat/tde-license-info from 6c423143f8 to 9b5a8c93ce 1 year ago
Poster
Owner

PR updated

PR updated
blu.256 commented 1 year ago
Collaborator

Looks good!

Looks good!
SlavekB requested changes 1 year ago
SlavekB left a comment
Owner

It is necessary to decide on the name of the instance × the name of the translation catalog.

It is necessary to decide on the name of the instance × the name of the translation catalog.
tde_l10n_create_template(
CATALOG "tdelicense"
SOURCES "*.cpp"
SlavekB commented 1 year ago
Owner

Because *.cpp files are part of the defaults, unless otherwise specified, then there is no need to explicitly determine SOURCES. In this case, it is also possible to omit CATALOG. So here it can be simplified to:

tde_l10n_create_template( "tdelicense" )

However, the catalog name must be the same as the instance name of the application. And because "TDELicense" is used for TDEAboutData in mainWindow.cpp, I assume that the same lower/UPPER case is needed.

Do you propose to change the name of the catalog or instance name of the application? Or in the application to add explicit determination of the name of the translation catalog?

Because `*.cpp` files are part of the defaults, unless otherwise specified, then there is no need to explicitly determine `SOURCES`. In this case, it is also possible to omit `CATALOG`. So here it can be simplified to: ``` tde_l10n_create_template( "tdelicense" ) ``` However, the catalog name must be the same as the instance name of the application. And because "TDELicense" is used for `TDEAboutData` in `mainWindow.cpp`, I assume that the same lower/UPPER case is needed. Do you propose to change the name of the catalog or instance name of the application? Or in the application to add explicit determination of the name of the translation catalog?
SlavekB marked this conversation as resolved
int main(int argc, char *argv[])
{
TDEAboutData aboutData("TDELicense", I18N_NOOP("TDE License"),
SlavekB commented 1 year ago
Owner

It is necessary to unify the name of the instance and the name of the catalog with translations or to use:

TDELocale::setMainCatalogue("tdelicense");
It is necessary to unify the name of the instance and the name of the catalog with translations or to use: ``` TDELocale::setMainCatalogue("tdelicense"); ```
Poster
Owner

Good, thanks for the feedback

Good, thanks for the feedback
SlavekB marked this conversation as resolved
MicheleC force-pushed feat/tde-license-info from 9b5a8c93ce to 3485003684 1 year ago
MicheleC force-pushed feat/tde-license-info from 3485003684 to b957aab3e4 1 year ago
SlavekB approved these changes 1 year ago
SlavekB left a comment
Owner

All looks good.

All looks good.
MicheleC merged commit b957aab3e4 into master 1 year ago
MicheleC deleted branch feat/tde-license-info 1 year ago

Reviewers

SlavekB approved these changes 1 year ago
The pull request has been merged as b957aab3e4.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Reference: TDE/tdebase#318
Loading…
There is no content yet.