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

Add server IP Chooser #735

Merged
merged 12 commits into from
Jan 5, 2022
Merged

Add server IP Chooser #735

merged 12 commits into from
Jan 5, 2022

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Dec 1, 2021

Fixes #322
Fixes #726
Needs kiwix/libkiwix#622

@juuz0 juuz0 requested a review from kelson42 December 1, 2021 07:46
@kelson42
Copy link
Collaborator

kelson42 commented Dec 4, 2021

@juuz0 Thank you, I will give a check.

@kelson42 kelson42 changed the title Add Ip Chooser Add IP Chooser Dec 4, 2021
@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 4, 2021

Updated for latest changes in kiwix/libkiwix#622
Good for a check now

@kelson42
Copy link
Collaborator

kelson42 commented Dec 4, 2021

@juuz0 It does not compile for me:

$ make
g++ -c -pipe -std=c++11 -I/usr/local/include/ -I/usr/local/include -I/usr/include/x86_64-linux-gnu -I/usr/include/p11-kit-1 -O2 -Wall -W -D_REENTRANT -fPIC -DVERSION=2.0.5 -DQT_DEPRECATED_WARNINGS -DQT_NO_DEBUG -DQT_WEBENGINEWIDGETS_LIB -DQT_WEBENGINECORE_LIB -DQT_QUICK_LIB -DQT_PRINTSUPPORT_LIB -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_WEBCHANNEL_LIB -DQT_QML_LIB -DQT_NETWORK_LIB -DQT_POSITIONING_LIB -DQT_CORE_LIB -I. -Isubprojects/QtSingleApplication/src -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWebEngineWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtWebEngineCore -isystem /usr/include/x86_64-linux-gnu/qt5/QtQuick -isystem /usr/include/x86_64-linux-gnu/qt5/QtPrintSupport -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtWebChannel -isystem /usr/include/x86_64-linux-gnu/qt5/QtQml -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr/include/x86_64-linux-gnu/qt5/QtPositioning -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -I. -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -o contenttypefilter.o src/contenttypefilter.cpp
In file included from src/contenttypefilter.cpp:2:
src/kiwixapp.h:142:21: error: cannot declare field ‘KiwixApp::m_nameMapper’ to be of abstract type ‘KiwixApp::NameMapperProxy’
  142 |     NameMapperProxy m_nameMapper;
      |                     ^~~~~~~~~~~~
src/kiwixapp.h:110:9: note:   because the following virtual functions are pure within ‘KiwixApp::NameMapperProxy’:
  110 |   class NameMapperProxy : public kiwix::NameMapper {
      |         ^~~~~~~~~~~~~~~
In file included from src/kiwixapp.h:18,
                 from src/contenttypefilter.cpp:2:
/usr/local/include/kiwix/name_mapper.h:36:25: note: 	‘virtual std::string kiwix::NameMapper::getNameForId(const string&) const’
   36 |     virtual std::string getNameForId(const std::string& id) const = 0;
      |                         ^~~~~~~~~~~~
/usr/local/include/kiwix/name_mapper.h:37:25: note: 	‘virtual std::string kiwix::NameMapper::getIdForName(const string&) const’
   37 |     virtual std::string getIdForName(const std::string& name) const = 0;
      |                         ^~~~~~~~~~~~
make: *** [Makefile:1017: contenttypefilter.o] Error 1

@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 4, 2021

@kelson42 because of changes in #732
Rebased now, should work

@kelson42
Copy link
Collaborator

kelson42 commented Dec 4, 2021

It compiles now, but I have a dubious warning:

src/settingsview.cpp: In member function ‘void SettingsView::init(int, int, const QString&)’:
src/settingsview.cpp:27:29: warning: unused parameter ‘port’ [-Wunused-parameter]
   27 | void SettingsView::init(int port, int zoomPercent, const QString &dir)
      |                         ~~~~^~~~

Otherwise looks pretty good:

  • One this is started, the inputtext should looks like before: no-border, read-only
  • The port input text should be for numbers only and only allowed beetween 0 and 65535
  • IPs should be sorted: 0.0.0.0 first, then 127.0.0.1, then all others sorted numerically
  • Chosen IP and port should be saved, so if the server is restarted, thenit restarts with the same configuration.

@kelson42 kelson42 changed the title Add IP Chooser Add server IP Chooser Dec 4, 2021
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Looks almost perfect, but needs a bit of beautiful.

@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 4, 2021

@kelson42 I agree on the beauty, haha. Will do them changes

@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 5, 2021

Updated @kelson42
Have still kept borders (although decreased size and used light-grey colour like in the invision designs) because without them it was looking weird. Let me know if they have to be removed :)
Followed all other points, I think ✔️

@juuz0 juuz0 requested a review from kelson42 December 5, 2021 06:00
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Few things left:

  • Default port should be 8080
  • 99999 input should not be possible at port number. Only values from 1 through 65535 are authorized.

@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 5, 2021

Both done

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Well done

@kelson42 kelson42 requested a review from mgautierfr December 5, 2021 11:59
@kelson42
Copy link
Collaborator

kelson42 commented Dec 5, 2021

@mgautierfr Can you please have a look to the code?

src/localkiwixserver.cpp Outdated Show resolved Hide resolved
@kelson42
Copy link
Collaborator

@juuz0 I have rebased and hopefuly, now that kiwix/libkiwix#622 has been merged, the CI will pass.

But I have remarked a small glitch:
image

Why in the selectbox, we have an empty entry, or at least a white margin, at the bottom of the IP list?

@juuz0
Copy link
Collaborator Author

juuz0 commented Dec 23, 2021

@kelson42 was an issue with padding, I removed the padding from there and decreased the Port Edit box's to 2px.

@kelson42
Copy link
Collaborator

kelson42 commented Jan 3, 2022

@mgautierfr We really need to move ahead with this PR, please review it quickly.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Only values from 1 through 65535 are authorized.

I wonder if we should go even further and prevent port < 1024.
Those ports are "Well-known ports" and most of the time (on linux at least), the user needs to be root to use them. We probably don't want to use them. (See https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers)

src/localkiwixserver.cpp Outdated Show resolved Hide resolved
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
@kelson42
Copy link
Collaborator

kelson42 commented Jan 3, 2022

@mgautierfr Not sure about that, port 80 is legit for example. I would avoid that discussion for the moment or at least pospone it in an other ticket.

@mgautierfr
Copy link
Member

port 80 is legit for a "real" server. Something running as a daemon and probably with some monitoring (and on linux you need root access)
I don't think it is the use case of kiwix-serve through kiwix-desktop. But we can postpone yes.

@juuz0 juuz0 requested a review from mgautierfr January 4, 2022 13:26
@mgautierfr
Copy link
Member

Look good to me. @juuz0 Please rebase/fixup before we merge.

@kelson42 kelson42 merged commit 789dfd5 into master Jan 5, 2022
@kelson42 kelson42 deleted the moreIPChoices branch January 5, 2022 14:22
@kelson42
Copy link
Collaborator

kelson42 commented Jan 5, 2022

@juuz0 Thx, great to see this PR finally merged and working so well!

@juuz0
Copy link
Collaborator Author

juuz0 commented Jan 5, 2022

Yay! :D

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.

Use new libkiwix server URL help to get the localhost URL Kiwix serve IP address is not configurable
3 participants