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

Implement portable mode #1179

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Implement portable mode #1179

merged 2 commits into from
Sep 10, 2024

Conversation

sgourdas
Copy link
Collaborator

@sgourdas sgourdas commented Aug 16, 2024

This adds a check for the .portable file besides the executable. If it exists all application files will be saved in the data directory.

Fixes #1166

@kelson42
Copy link
Collaborator

@sgourdas What is exactly the status here? It is mark ready to review, but still WIP and no reviewer? It's unclear.

@sgourdas sgourdas marked this pull request as draft August 19, 2024 13:07
@sgourdas
Copy link
Collaborator Author

Changing to draft since getDataDirectory is not merged yet. I have finished work on the others.

@kelson42
Copy link
Collaborator

@sgourdas If there is a blocker, please write it clearly. Now that the data directory PR are merge, does that mean this PR is ready to review?
@sgourdas Please rebase an fix conflict.

@sgourdas
Copy link
Collaborator Author

Rebased with main and completed changes. Ready for review.

@sgourdas sgourdas marked this pull request as ready for review August 23, 2024 20:22
src/kiwixapp.cpp Show resolved Hide resolved
src/kiwixapp.cpp Outdated Show resolved Hide resolved
src/settingsmanager.cpp Outdated Show resolved Hide resolved
@veloman-yunkan
Copy link
Collaborator

WIP qualifier is no longer pertinent, I guess

@sgourdas sgourdas changed the title WIP: Implement portable mode Implement portable mode Aug 30, 2024
@kelson42
Copy link
Collaborator

kelson42 commented Sep 1, 2024

@sgourdas Sorry for the lack of feedback here, will look ASAP

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.

I have checked finally your feature. This is on the good way, but it seems that the configuration is still take from the older "system" conf.
image

I see also no kiwix-desktop.conf in the data directory. Please verify that all configuration files from https://github.com/kiwix/kiwix-desktop/wiki/Data-and-Configuration-Files are read/write locally. Please update https://github.com/kiwix/kiwix-desktop/wiki/Data-and-Configuration-Files with a new "portable" column as well.

@kelson42
Copy link
Collaborator

kelson42 commented Sep 7, 2024

@sgourdas Any update?

@sgourdas
Copy link
Collaborator Author

sgourdas commented Sep 7, 2024

Sorry for the delay. I will update ASAP.

@kelson42
Copy link
Collaborator

kelson42 commented Sep 9, 2024

@sgourdas Please ensure this PR fixes #460 properly

@sgourdas sgourdas force-pushed the feature/portable-kiwix branch 3 times, most recently from 420e1d0 to 862412a Compare September 9, 2024 18:12
@sgourdas
Copy link
Collaborator Author

sgourdas commented Sep 9, 2024

Please update https://github.com/kiwix/kiwix-desktop/wiki/Data-and-Configuration-Files with a new "portable" column as well.

@kelson42 what should the "portable" column have as information?

@kelson42
Copy link
Collaborator

kelson42 commented Sep 9, 2024

Please update https://github.com/kiwix/kiwix-desktop/wiki/Data-and-Configuration-Files with a new "portable" column as well.

@kelson42 what should the "portable" column have as information?

The relative paths where the file are read/written

@sgourdas
Copy link
Collaborator Author

sgourdas commented Sep 9, 2024

Please update https://github.com/kiwix/kiwix-desktop/wiki/Data-and-Configuration-Files with a new "portable" column as well.

@kelson42 what should the "portable" column have as information?

The relative paths where the file are read/written

I have set it up so in portable mode all files are written in the data folder besides the executable. Since it is all the same maybe just a note under the table would be enough?

@sgourdas
Copy link
Collaborator Author

sgourdas commented Sep 9, 2024

@veloman-yunkan I added support also for the download directory and .conf file to work appropriately in portable mode.

@kelson42 kelson42 self-requested a review September 9, 2024 19:43
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.

To me, this PR seems to perfctly implement #1166

@veloman-yunkan This is ready for the code review

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Regarding the current changes I have only a couple of insignificant proposals for improvement.

But regarding the portable functionality I have the following questions:

  1. Should we allow the user to change/set the download and monitored directories in portable mode?
  2. Should we allow adding ZIM files from outside the USB drive to the library in portable mode?

src/settingsmanager.h Outdated Show resolved Hide resolved
src/settingsmanager.h Outdated Show resolved Hide resolved
@sgourdas
Copy link
Collaborator Author

But regarding the portable functionality I have the following questions:

  1. Should we allow the user to change/set the download and monitored directories in portable mode?
  2. Should we allow adding ZIM files from outside the USB drive to the library in portable mode?

I was thinking about this too and how strict we should be with the portable mode.

IMO we should not be very restrictive and let the user choose. But, with the current implementation, if the download directory is changed and mode is portable, it would be set back to the data folder on app restart, which I dont think is proper, but adds some complexity if we want to tackle.

@veloman-yunkan
Copy link
Collaborator

But, with the current implementation, if the download directory is changed and mode is portable, it would be set back to the data folder on app restart

How come it works like that? Isn't the download directory setting saved in the kiwix-desktop.session file?

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM.

@sgourdas
Copy link
Collaborator Author

But, with the current implementation, if the download directory is changed and mode is portable, it would be set back to the data folder on app restart

How come it works like that? Isn't the download directory setting saved in the kiwix-desktop.session file?

It does usually, but not in portable mode as per this PR.

settingsmanager.cpp:192

@kelson42
Copy link
Collaborator

kelson42 commented Sep 10, 2024

1. Should we allow the user to change/set the download and monitored directories in portable mode?

Why not? But to me this is something different: I have created #1203

I believe that per default, the data directory should be monitored, see my comment.

2. Should we allow adding ZIM files from outside the USB drive to the library in portable mode?

Why, not? At the end this is also a topic which is relevant for #1203 IMHO

@kelson42 kelson42 merged commit 5e5bcdc into main Sep 10, 2024
6 checks passed
@kelson42 kelson42 deleted the feature/portable-kiwix branch September 10, 2024 09:58
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.

Portable Kiwix, allow to custom all local storage/configuration locations
3 participants