Daring Daffodils #20
Daring Daffodils #20
Conversation
…st, Also made a full python browser extension replacing the older js The extension would be only accessible via localhost, so the server and mobile page need to run in the local machine Also avoided browser_extension/runtime from ruff check
… the movement ratio Hence, could be used for any screen size browsers
…part Add logging statement for FastAPI App with mobile page qr code 1. Mode activation after 10 seconds of in-activity, after that random mode selection for a random period of time 2. It would work after page redirection too
Linting and formatting has been skipped, would be finished in the later part
Here the easter-eggs would be hiding hilarious websites and YouTube videos link inside the webpage, so when there is an inactivity some modes would get activated, and they would click these links randomly on the way
minor bug fixes on text copy extension popup on click remove unwanted files
feat: Change UI for the mobile page and icon for the mouse Graffiti themed with minimal js has been used for mobile_page
feat: Add easter-eggs on the sites loading
feat: Add Makefile and remove the font on mobile page
Made utils directory from Chrome extension
Fix/linting and formatting
Completed the Feature and Installation Step
Feat/add documentation
Feat: Add explanation about usage
Feat: Add why wrong tool for the job doc
Feat/add documentation
There was a problem hiding this comment.
The Overall Picture
Very nicely done project! The setup between computer and my phone was very nicely done. The overall structure of the project made it easy to review. It's a very fun idea that was fun to demo and test. The code quality throughout the project is a bit varying, but is solid--more on that below and throughout the comments. The project layout makes it easy to navigate and review. My one comment there is that I think the utils/ folder can be better named.
README & Documentation
The README itself does a great job of showcasing the project and also providing good installation instructions. Some of the comments in the code aren't necessary as the code speaks for itself, while other parts are denser and could use some comments to clarify what the intended behavior is. The docstrings in your code were informative and helpful, nicely done there.
Commits
The commit history looks good, with appropriate and informative names. There aren't a lot of commits with some of them being quite large. Something you can't get around this after some minimum viable product stage, because you want to ensure every commit can properly run. However, looking at some of them I think there were a few opportunities for more atomic commits.
Code Quality
Overall, the code was easy to follow and read. The browser extension portion has a lot of globals mutating state. It made it difficult to follow what was being mutated where and how the control logic flowed. I think there can be better solutions to that.
| from js import document | ||
|
|
||
|
|
||
| def fetch_easter_eggs(): # Find the element by ID |
There was a problem hiding this comment.
This comment doesn't seem super necessary. The docstring is thorough and the document.getElementById(...) function is fairly clear.
The typehint for this function does not match what's in the doc string.
|
|
||
| if el: | ||
| rect = el.getBoundingClientRect() | ||
| easter_eggs_coordinates.append([rect.x, rect.y]) |
There was a problem hiding this comment.
These coordinates seem to not change during the runtime of the program via mutation. It would make a bit more sense to turn this into a tuple since it'll always be a coordinate pair.
That'll also change the return type to list[tuple[float, float]].
| # Initialize fake cursor element | ||
| fake_cursor = create_fake_cursor() | ||
|
|
||
| # Attach WebSocket event listeners | ||
| ws.addEventListener("open", create_proxy(onopen)) | ||
| ws.addEventListener("message", create_proxy(onmessage)) | ||
| ws.addEventListener("close", create_proxy(onclose)) |
There was a problem hiding this comment.
Although not explicitly necessary, I would wrap this in an if-name-main guard to ensure it only runs when this file is called directly, which I believe it is? If it's not then disregard.
| # Start idle tracking immediately | ||
| reset_inactivity_timer() |
There was a problem hiding this comment.
I would move this down to the rest of the primary functions and not have it nested inbetween function defintions.
| - 'shadow': cursor flickers (visible/hidden) | ||
| It may also click on clickable elements or snap to Easter egg positions. | ||
| """ | ||
| global WANDERING, WANDERING_PROXY |
There was a problem hiding this comment.
You have a lot of globals in this file. A lot of globals mutating state in a lot of different places that makes it hard to track.
If you have state like this with methods that alter the state, this seems like a good opportunity to use a class.
It would make it clearer which variables are stateful, which ones are local the function and do not need to maintain state, and which ones are not in the stateful-class at all.
| import io | ||
| import sys | ||
|
|
||
| buf = io.StringIO() | ||
| sys_stdout = sys.stdout | ||
| sys.stdout = buf | ||
| qr.print_ascii(invert=True) | ||
| sys.stdout = sys_stdout |
There was a problem hiding this comment.
I don't think these imports need to be kept in the function, they can be moved to the top level. They're not particularly expensive imports and shouldn't clash with anything else.
Additionally a comment here about the intent of this code snippet would be beneficial.
| - Normalizes end coordinates relative to browser dimensions. | ||
| - Sends gesture data via sendCoords to the WebSocket server. | ||
| """ | ||
| global PRESS_TIMER |
There was a problem hiding this comment.
I don't believe this global is necessary because you aren't changing the PRESS_TIMER value. Functions can read from the global/module scope without needing the global keyword. It only needs the global keyword if you want to re-assign to a new value.
| touch_area = document.getElementById("touchArea") | ||
| touch_area.addEventListener("touchstart", create_proxy(touch_start), {"passive": True}) | ||
| touch_area.addEventListener("touchmove", create_proxy(touch_move), {"passive": True}) | ||
| touch_area.addEventListener("touchend", create_proxy(touch_end), {"passive": True}) |
There was a problem hiding this comment.
I would move these below all of the function definitions so they don't get lost.
| make | ||
| ``` | ||
| - Windows | ||
| - By default, Windows does not include GNU Make. Install via [this](https://www.msys2.org/) |
There was a problem hiding this comment.
I would recommend for Windows users to use GnuWin Make instead. It's a bit simpler to install and use than msys2.
| try: | ||
| el.remove() | ||
| finally: | ||
| cb = holder.pop("cb", None) |
There was a problem hiding this comment.
I think a variable of callback would be a bit clearer. Until you see the comment about the persistent JS callback further below, I had no idea what cb was.
No description provided.