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.

Sign up for GitHub

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

Jump to bottom

Fix settings #41

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix settings #41

wants to merge 5 commits into from

Conversation

sysadminstory
Copy link

Hello !

This is a fix for the file association settings not working #27 :

  • switched the settings saving URL to a controller
  • Created a panel only for epubreader in the personal settings section

I never worked on any Nextcloud app before, so this needs to be reviewed !

I did test it only on a test Nexctloud instance !

@sysadminstory
Initial fix for File Association preference
3c4bfb6 
This is a initial fix to permit user to change the file association
preferences again.
@sysadminstory
Cleanup old files and update comments
13d4946 
Change the settings url to be mor consistant with the others urls of the
app.

Better feedback if the saving of settings did work or not.

Use the Framework to get the settings value, instead of using the $_POST
array.
This was referenced Jan 22, 2022
Open
Open
@sysadminstory
Fix the settings controller parameters
9f1250a 
The Settings controller parameters were did not have a type, and
prevented them from being automatically injected
@shyzus
Copy link

shyzus commented Jan 24, 2022 •
edited
Loading

First off, thank you for your work on this fix. I have used this app with the fix applied and it works very well. One thing to note would be the removal of personal.php. If you could remove this it would be great. The way it currently stands there are two setting pages by removing this one it looks a lot better.

Look forward to seeing this merged. 👍

@sysadminstory
Copy link
Author

sysadminstory commented Jan 24, 2022 •
edited
Loading

Thanks for pointing this : I did not know where the "old" settings page were defined !

@sysadminstory
Remove old settings file
69d4859 
Remove the 'personal.php' file to remove the duplicate personal settings
page
@ummon29
Copy link

ummon29 commented Jan 30, 2022

Thank you for our work on this.
I tried both of your Fixes (#41, #42) an they mostly seem to work fine. The only issue I encountered is that i am now unable to open cbz/cbr files. They download but never start to extract. Switching back to 1.4.7 without your fixes they open up fine. epub and pdf are working fine. I use Nextcoud 23.0.0 with nginex.
Does anyone else have this problem? I'm hoping to have everything in your fixes applied the right way.

@sysadminstory
Copy link
Author

The fixes in #42 are a work in progress pull request so this can not be considered as "working", To be honest I'm lost in all those JavaScript loading each other ...

@shyzus
Copy link

shyzus commented Mar 10, 2022 •
edited
Loading

After some more use I did some deeper digging after certain errors started popping up regarding the app being unable to find the file personal.php.

It appears that it is still required and being loaded and sometimes cached in the /ajax/ directory. If this file is not present in the root directory of the app it will throw an error in the nextcloud logs, however if it was still cached in the ajax directory it will continue to operate. Once that file is also removed then the app no longer works properly when it comes to its settings. I did some digging and found a temporary solution for this. First restoring personal.php back to its original location in the root directory as it was before commit: 9f1250a. Then modifying app.php by removing the line \OCP\App::registerPersonal('epubreader', 'personal'); This prevents it from being registered under the personal section in the additional settings submenu. This issue may also resolve @ummon29 issue since the preferences may have been not working if they didn't have a cached version of personal.php available.

@ummon29
Copy link

ummon29 commented Mar 16, 2022

I tried again with the personal.php in it's previous location. My app.php was already modified. The behavior is the same as before. Epub is working fine. cbr/cbz are downloaded but never start to extract.

@sysadminstory sysadminstory mentioned this pull request Mar 22, 2022
Open
@sysadminstory
Update Route in personal.js
6eadc60 
It seems personal.js is used in the settings too, so i changed the URL
there too.
@sysadminstory
Copy link
Author

sysadminstory commented Mar 22, 2022 •
edited
Loading

I got this issue fixed : I forgot to update the personal.js file with he new route !

@devnoname120
Copy link

@sysadminstory Is anything missing for merging this pull request?

@devnoname120 devnoname120 mentioned this pull request Jul 30, 2023
Open
devnoname120 pushed a commit to devnoname120/epubviewer that referenced this pull request Jul 30, 2023
@sysadminstory @devnoname120
Fix File Association preference
2ce6b85 
Original PR: e-alfred#41

This fix repairs the installation of the app that was failing: `Class "OCP\App" not found"`.

It also allows user to change the file association preferences again.
@sysadminstory
Copy link
Author

@sysadminstory Is anything missing for merging this pull request?

Well, on my point of view, no, but the owner of this repo seems to be inactive :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sysadminstory @shyzus @ummon29 @devnoname120