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

Split HTML and binary #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Split HTML and binary #17

wants to merge 1 commit into from

Conversation

torwag
Copy link

@torwag torwag commented Mar 31, 2020

In addition, I refactored the html code as requested.

  1. The scrollbars are gone
  2. The animation of the icons are faster now
  3. Zoom by mouse is added (Ctrl+Mousewheel)

It requires now to copy both files (bin and html) to the reMarkable.

@torwag
Copy link
Author

torwag commented Mar 31, 2020

I could not fix the rotation animation after the fifth rotation, as it seems to be done not on purpose.
The readme need to be changed as it requires now two files to be uploaded to the rM (bin and html).
I could not find whether it is necessary to close the file after it was opened.

@torwag
Copy link
Author

torwag commented Apr 3, 2020

Any thoughts or wishes?
I could add a "simple.html' with the original service site and another pr could add a cli-parameter to be able to select the desired html file.

@Merovius
Copy link
Owner

Merovius commented Apr 3, 2020

  • I don't like having to copy the file separately. It should really be embedded.
  • There's also no need to use io.WriteString - use w.Write directly. It saves an allocation (converting from []byte to string or vice-versa requires an allocation)
  • log.Fatal in an HTTP Handler is a no-no. Luckily, if the file is embedded anyway, it doesn't matter.
  • I'd like the commit-message to specifically mention @bordaigorl. Ideally, they would also comment and sign off here. It's their code after all and it should be properly attributed.
  • The file is also not run through gofmt.

I wanted to decide on an embedding tool and then do the changes myself, but I didn't get around to it yet (as you might have noticed, I'm not super disciplined as far as working on this project is concerned :) ).

@torwag
Copy link
Author

torwag commented Apr 3, 2020

Thanks for the feedback.
First, I have to say, I never touched Go before, thus, this PR was a flight without reading the safety manual first.

I don't like having to copy the file separately. It should really be embedded.

Well my thought on this was, that development can be split up more easily, enabling some html+css wizards (their is a certain commentar about css in the source file ;) ), to do their stuff and hence, get some more people involved. Furthermore, different people might have different needs and the easiest would be to enable them to select which file they would like to serve.

There's also no need to use io.WriteString - use w.Write directly. It saves an allocation (converting from []byte to string or vice-versa requires an allocation)

Well, as I said, I am a complete Go beginner. I simply used what I figured out first.

log.Fatal in an HTTP Handler is a no-no. Luckily, if the file is embedded anyway, it doesn't matter.

Again, I just found a lot of examples that did it that way. I thought if the program can't find the file it would be right to terminate the program.

I'd like the commit-message to specifically mention @bordaigorl. Ideally, they would also comment and sign off here. It's their code after all and it should be properly attributed.

Definitely agree, credits go where credits belong. In the issue #4 , was some good start, but finally no movement. I used #4 to create my own version a while ago with all the whistles and bells, including the zoom by mouse. I added the splitting of the files as this enabled me to tinker with the html and the css part without the need to recompile everything over and over. Thus, I thought I go the extra mile and give it back to the community and see how it goes.

The file is also not run through gofmt.

Another, I never used go before thing, it includes I never heard of gofmt before ;) Will look into it.

I wanted to decide on an embedding tool and then do the changes myself, but I didn't get around to it yet (as you might have noticed, I'm not super disciplined as far as working on this project is concerned :) ).

My life is not depending on this PR either, as my own private version works for me. I took this as an academic adventure and excuse to delve into Go and see how it goes ;) It furthermore creates noise on github, pushing people into believing that this project is not dead ;)

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.

None yet

2 participants