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

Display the kolibri link address to the system tray message - alternate branch #110

Merged

Conversation

cpauya
Copy link
Contributor

@cpauya cpauya commented Jul 5, 2019

Supersedes #106

Addresses this issue: #104

Testing PR: Grab the unsigned Windows installer here learningequality/kolibri#5743

Note: WIP, needs to create a testing PR for this, please don't merge yet.

@mrpau-richard
Copy link
Contributor

I tested this on Windows 7 and it's working as expected.

Screen Shot 2019-07-08 at 3 22 56 PM

@benjaoming
Copy link
Contributor

There are 3 binary files in this PR, are they build artifacts?

@mrpau-julius
Copy link

mrpau-julius commented Jul 9, 2019

@benjaoming The Resource.rc and Ressource.h are a text file, not a binary. I don't know why GitHub treated that a binary. We discussed the kolibri.exe to create a separate issue and find ways on how to solve it. @cpauya agree that all the files on the repository must be a text or image, not binary files.

@cpauya
Copy link
Contributor Author

cpauya commented Jul 9, 2019

Good question @benjaoming 😸 and to answer it: No they are not build artifacts. 😿

It's indeed weird Github is treating the .rc and .h files as binaries. Their contents are in plain text as you can see here:

However, the Kolibri.exe being in the repo is questionable.

As much as possible, our repo should contain only sources, images (which are binary files too), maybe reference libraries (.dll, .lib, etc) and NO binary executables as much as possible. This is a remnant of the way things worked with KA-Lite for Windows before: https://github.com/learningequality/ka-lite-installers/tree/master/windows/gui-packed

So yes, we must create another issue for removing Kolibri.exe from the source repo @mrpau-julius and have that auto-generated during build.

@cpauya
Copy link
Contributor Author

cpauya commented Jul 9, 2019

Ha! The binary treatment apparently has possibly lots of reasons why as per this StackOverflow post: 😸

  • file encoding in UTF-8 or UTF-16
  • long lines of strings in text file
  • Unicode characters inside the text file
  • etc

@@ -216,6 +226,12 @@ void serverStartingMsg() {

void checkServerThread()
{
wchar_t msgString[120];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 120 character-limiting index seems unusual. Although I think it's trivial for now but let's take note in case the notification will take longer than 120 characters.

@benjaoming
Copy link
Contributor

benjaoming commented Jul 9, 2019

@cpauya thanks for explaining and looking into why Github gives this wrong classification of the two files - one more question: how is kolibri.exe built if it's not a build artifact?

@cpauya
Copy link
Contributor Author

cpauya commented Jul 9, 2019 via email

@cpauya
Copy link
Contributor Author

cpauya commented Jul 11, 2019

Merging this now.

Further testing may be done in this Testing PR for milestone 1.3.1 at learningequality/kolibri#5749

@cpauya cpauya closed this Jul 11, 2019
@cpauya cpauya reopened this Jul 11, 2019
@cpauya cpauya merged commit a1c3a6c into develop Jul 11, 2019
@cpauya cpauya deleted the Display-the-Kolibri-link-address-to-the-system-tray-message branch July 11, 2019 05:29
@benjaoming
Copy link
Contributor

@cpauya Is it not possible to build this kolibri.exe in a Wine environment?

@indirectlylit
Copy link
Contributor

indirectlylit commented Jul 12, 2019

@benjaoming we opened up a new issue for kolibri.exe since it was unrelated to this PR: #112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants