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

Feat big gui update #16

Closed
wants to merge 32 commits into from
Closed

Feat big gui update #16

wants to merge 32 commits into from

Conversation

PhilippMundhenk
Copy link
Owner

testing and working in the changes from #15

philippderdiedas and others added 27 commits February 19, 2024 02:16
Remove . and .. because these are not files
followup to previous commit
don't list non existing files
Add polling functionality
Add scanning animation (very proud ;) )
Fix css comment syntax
update Dockerfile, stripped docker image down
remove sudo from dependencies
add status backend endpoint
add ping status to display scanner offline status in web ui
add loading spinner to selected button in web ui
add more verbosity to runScanner.sh
rename env variables for consistency
name was always root before not editable with USERNAME env var
    update Dockerfile

    use lighty-enable-mod to enable lighttpd mods
    added mod_access config

    change backend structure

    create non servable lib folder
    move config.php to lib
    add helper functions in lib
    add subpath checking for bad path prevention
remove unused lines in scripts
@PhilippMundhenk
Copy link
Owner Author

PhilippMundhenk commented Feb 25, 2024

@philippderdiedas: It didn't quite work out of the box. I adapted and corrected a number of things (variable handling, download path, etc.), added a ping interval, implemented defaults, etc. See commits above for details.

  • No buttons existing, no matter which label I set (or don't set any), buttons do not show in the GUI.
  • I also added an "unknown" status, in case the ping is disabled, GUI would otherwise show "offline", which might not be correct. But I guess something is wrong there, as it now always shows "unknown".

Maybe you can take a look, as well. I am not very well versed on the GUI side of things and a lot has changed. You might be faster in finding these errors.
You can test the image from this branch by using image: ghcr.io/philippmundhenk/brotherscannerdocker:feat_bigGuiUpdate

Good news: I tested scanning via buttons, etc. also and this works fine. So the tedious user issues should not be a problem.

@philippderdiedas
Copy link
Collaborator

philippderdiedas commented Feb 25, 2024

oof, i think you broke the runScanner.sh. when you do if [ "${WEBSERVER_ENABLE:-$WEBSERVER_DEFAULT}" ] it will always be true because when WEBSERVER_ENABLE is unset, it will use WEBSERVER_DEFAULT as value and because that is a non zero length string ("true") it computes to true

@philippderdiedas
Copy link
Collaborator

i think i switched the default value for WEBSERVER because i misread that somehow, thats why my variable is called WEBSERVER_ENABLED impyling if not set the default is false, would call it WEBSERVER_DISABLED then

@philippderdiedas
Copy link
Collaborator

philippderdiedas commented Feb 25, 2024

oof, i think you broke the runScanner.sh. when you do if [ "${WEBSERVER_ENABLE:-$WEBSERVER_DEFAULT}" ] it will always be true because when WEBSERVER_ENABLE is unset, it will use WEBSERVER_DEFAULT as value which is a non zero length string ("true") it computes to true

and thats why all labels are ""

for more info shell parameter expansion

@PhilippMundhenk
Copy link
Owner Author

PhilippMundhenk commented Feb 25, 2024

WEBSERVER_ENABLE was never unset though, as it is set to at least empty string in the backward compatibility section (which had a different issue, but this is fixed now, as well). But you are right, even if set to "false" it would turn on, as it is a string with length > 0. Added an explicit comparison in the recent commits. Tested and works.

Anyway, all of this is doesnt solve the mystery of the missing buttons on the page...

@philippderdiedas
Copy link
Collaborator

philippderdiedas commented Feb 25, 2024

Do you have a compose file for reference?
because mine are showing up (but ui is blocked because ping disabled, thats another problem)

@PhilippMundhenk
Copy link
Owner Author

Aaahh..maybe the UI block is the issue...

I am currently focusing on backward compat checks with something like this:

version: '3'

services:
    brother-scanner:
        image: ghcr.io/philippmundhenk/brotherscannerdocker:feat_bigGuiUpdate
        volumes:
            - /path/to/scans:/scans 
        ports:
            - 33355:33355
            - 54925:54925/udp
            - 54921:54921
            - 161:161/udp
        environment:
            - NAME=Scanner
            - MODEL=MFC-L2700DW
            - IPADDRESS=192.168.123.30
            - UID=1111 # note: network mount needs to have correct permissions!
            - GID=1111 # note: network mount needs to have correct permissions!
            - TZ=Europe/Berlin
            - WEBSERVER=true
            - PORT=33355
            - DISABLE_GUI_SCANTOIMAGE=true
            - DISABLE_GUI_SCANTOOCR=true
            - RENAME_GUI_SCANTOFILE="Scan front pages"
            - RENAME_GUI_SCANTOEMAIL="Scan rear pages"
            - HOST_IPADDRESS=192.168.123.123
            
        restart: unless-stopped

@PhilippMundhenk
Copy link
Owner Author

PhilippMundhenk commented Feb 25, 2024

Excellent! That got me on the right track, thank you! I missed out a semicolon in status.php when adding the "unknown" status.

Buttons are there, scan can be triggered, nice! Unfortunately, there still seem to be some permission issues, when scanning from the GUI:

/opt/brother/scanner/shell_env.txt: line 29: declare: UID: readonly variable
mkdir: cannot create directory '/scans/2024-02-25-20-49-42': Permission denied

Any ideas? If not, I will take a look in te coming days. I am very willing to take up all these changes now. You put a lot of effort in and it shows! I really like the nicer GUI, the blocking, the scanning animation, etc. Not to mention all the cleaning of my hacked scripts. Thank you very much!

@PhilippMundhenk
Copy link
Owner Author

Somehow this dropped off my radar for quite some time and only came back up, when #24 came up. Since that one works as a drop in replacement, we have merged that to master and I consider this superseded

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.

2 participants