Dynamic Typists #2

janine9vn · 2023-09-28T15:42:59Z

No description provided.

Co-authored-by: Oliver Wilkes <[email protected]> Co-authored-by: Thijs Franck <[email protected]>

Co-authored-by: Istalantar <[email protected]>

Co-authored-by: Thijs <[email protected]> Co-authored-by: Oliver Wilkes <[email protected]>

Co-authored-by: Oliver Wilkes <[email protected]> Co-authored-by: Thijs <[email protected]> Co-authored-by: maxyodedara5 <[email protected]>

Co-authored-by: Thijs <[email protected]>

Co-authored-by: Thijs Franck <[email protected]>

…047fcb8' git-subtree-dir: dynamic-typists git-subtree-mainline: 4bae50a git-subtree-split: 0b38e8c

janine9vn

Hello! Thanks for your patience as we get this review sorted out in GH format.

Documentation & README

The documentation throughout this project is of a very high level and impressive. This project would have been much more difficult to review with out. The READMEs in each section are helpful for explaining the layout and some decisions made. In some cases it may even be too thorough and in-depth, although I don't think you'll find anyone complaining. There are still a handful of instances that could use a bit more explanation or comments, but by and far the documentation for this is very well done.

The module docstrings can either be removed or be expanded on. Right now they're sitting in a bit of an uncertain zone of having some basic information, but not enough to prevent someone from going elsewhere to get a deeper-than-surface-level understanding of what the module is doing.

Also shoutout for the inclusion of the js.pyi, it was helpful for this review and to stop my IDE from complaining.

Code Quality & Organization

The code is also very well done. There are a few instances where I think some things are unnecessarily complicated, a little too long for a single function, or there are nested functions that can be broken apart / simplified. I'm not familiar with pyodide in the slightest, but the this code was still (mostly) easy to reason about.

That being said, I did get a bit lost in the sauce trying to make sense and understand the different controllers. That was one area where I do wish there a bit more documentation explaining what controller is used where and why. For instance, the drag_drop_grid_controller and the image_grid_controller sound like they do the exact same thing but worded slightly differently. Turns out that image_grid_controller does user drag_drop_grid_controller, but it took a little bit to trace that back and keep track of it all. So either a combination of a slight re-organization and slight-renaming or a document/diagram illustrating how they're all used could go a long way.

Commits

21 commits is not a lot of commits for a project this size. That number alone, looking at the scale and scope of this project, implies that some commits are likely bigger than they should be. Digging into it a bit, the true number of commits is lost at the Pull Request stage. The individual commit information is retained in the pull request, but when I'm viewing it locally there are just these giant commits that do a lot that I can't break apart into more manageable peices. So I recommend following an atomic git commit approach in the future. I like recommending Make Atomic Git Commits for further reading about atomic git commits and why they are beneficial.

Summary

Overall, this project is very well done. It has really solid code quality, excellent documentation, and the actual execution of it is impressive. Good job y'all~

janine9vn · 2023-10-16T21:39:18Z

dynamic-typists/app/README.md

This type of documentation is very impressive and thorough. Having READMEs in the differnt subfolders to dive deeper into the details of each is very much appreciated.

janine9vn · 2023-10-16T22:18:32Z

dynamic-typists/app/picture.py

+        # chose contrast based on the originals image brightness
+        watermark_contrast = int(self._get_brightness() * 0.8)


This is nicely done and a helpful comment as well.

janine9vn · 2023-10-16T22:19:31Z

dynamic-typists/app/scrambler.py

@@ -0,0 +1,145 @@
+"""Scramble functions for scrambling images in different ways."""


I don't think a comment like this at the top of files is necessary. The name of this file and the functions contained within are enough imo.

janine9vn · 2023-10-16T22:34:19Z

dynamic-typists/app/scrambler.py

+    co_ordinates = [
+        (0, 0, 512, 512),
+        (512, 0, 1024, 512),
+        (0, 512, 512, 1024),
+        (512, 512, 1024, 1024),
+    ]


This seems fairly hardcoded for specific image sizes. Is the picture given guaranteed to be provided in the correct size?

janine9vn · 2023-10-16T22:35:29Z

dynamic-typists/app/server.py

+    match scrambler:
+        case "rows":
+            scramble_rows(picture)
+        case "grid":
+            scramble_grid(picture)
+        case "circle":
+            scramble_circle(picture)


Not the most ideal usage of match-case, but I like the readability it provides here even if it's equivalent to if-elif statements.

Preferred match for this since the performance impact is negligible

janine9vn · 2023-10-19T00:31:36Z

dynamic-typists/app/solver.py

+    num_tiles = len(picture.tiles)
+    for ring_position in range(1, num_tiles):


I think this can be condensed, having num_tiles as its own variable isn't doing much here.

janine9vn · 2023-10-19T00:36:44Z

dynamic-typists/frontend/controllers/rotating_images_controller.py

+        background_element.classList.add("background-image")
+        self.root.appendChild(background_element)
+
+        for image in reversed(rotatable):


Why reversed here? It's not clear why this needs to be reversed.

This definitely requires some explanation.

We receive images from the API in the order: [background_image, smallest_ring, ..., largest_ring]. In HTML rendering, the last element in the list appears on top. Since each ring must remain selectable for user rotation, and considering the transparent part of a ring is still part of its clickable element, we need to ensure that smaller rings aren't obscured by larger ones. Thus, by reversing the list, we ensure smaller rings are rendered last (and therefore on top), keeping them selectable.

janine9vn · 2023-10-19T00:38:38Z

dynamic-typists/frontend/controllers/rotating_images_controller.py

+        List[float]
+            List of rotation values in degrees.
+        """
+        return [controller.current_rotation for controller in reversed(self._controllers)]


It seems the order of the list of rotation values is important, but it's not clear which direction it should be or which direction it's starting in.

Because we create the controllers in reverse order, we need to again reverse the order here to build the solution that the API expects.

janine9vn · 2023-10-19T00:44:02Z

dynamic-typists/frontend/main.py

+
+    add_event_listener(confirm_button, "click", handle_post_solution)  # type: ignore
+    add_event_listener(refresh_button, "click", handle_load_captcha)  # type: ignore
+    add_event_listener(restart_button, "click", lambda _: app.reset())


Rather than have a lambda to "eat" the argument, I would rather the reset() function itself in app.py discard/ignore all arguments given to it. reset() is only called here, so it feels a bit odd to know it will be passed an unnecessary event and that you have to go out of your way to discard it with a lambda.

janine9vn · 2023-10-19T00:48:53Z

dynamic-typists/frontend/main.py

+        The function utilizes a nonlocal variable `is_loading_captcha` to prevent multiple
+        concurrent requests when the user repeatedly clicks the button. This ensures that the
+        application does not request another captcha until the previous one has been loaded.


Smart choice!

thijsfranck and others added 22 commits August 28, 2023 21:01

Initial commit

a69e5e2

Environment setup (#2)

1ca6592

Co-authored-by: Oliver Wilkes <[email protected]> Co-authored-by: Thijs Franck <[email protected]>

Image resources for scrambling (#5)

8a39aa7

Update ruff rule ignores (#7)

bcbb560

add class templates (#4)

992bc01

Add row scramble feature (#6)

e7ee03c

Tile scrambler (#8)

3b80936

Co-authored-by: Istalantar <[email protected]>

add a mockup of the captcha widget (#3)

2631f69

Backend api (#15)

d4b3b0e

Co-authored-by: Thijs <[email protected]> Co-authored-by: Oliver Wilkes <[email protected]>

circle scrambler base (#13)

2603c44

Co-authored-by: Oliver Wilkes <[email protected]> Co-authored-by: Thijs <[email protected]> Co-authored-by: maxyodedara5 <[email protected]>

removed scripts.js since it's not used anymore (#18)

61b64de

Fix frontend type issues (#19)

da2fc47

Co-authored-by: Thijs <[email protected]>

Solve circle (#20)

c0e0508

Co-authored-by: Thijs <[email protected]>

Add watermark feature (#17)

6701272

Co-authored-by: Thijs <[email protected]>

Adding images (#23)

129cbb9

Capitalized c (#24)

c11ac72

Cleanup: remove unused code (#25)

f45feac

fix unused parameter (#26)

2a72cee

docs fixup (#27)

f4c06f3

Convert protocol.__all__ to a tuple (#28)

abdf594

Documentation (#29)

0b38e8c

Co-authored-by: Thijs Franck <[email protected]>

Add 'dynamic-typists/' from commit '0b38e8c7207927c41193444d95a64d150…

f202933

…047fcb8' git-subtree-dir: dynamic-typists git-subtree-mainline: 4bae50a git-subtree-split: 0b38e8c

janine9vn commented Oct 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Typists #2

Dynamic Typists #2

janine9vn commented Sep 28, 2023

janine9vn left a comment

janine9vn Oct 16, 2023

janine9vn Oct 16, 2023

janine9vn Oct 16, 2023

janine9vn Oct 16, 2023

janine9vn Oct 16, 2023

ooliver1 Oct 19, 2023

janine9vn Oct 19, 2023

janine9vn Oct 19, 2023

thijs-franck Oct 19, 2023

janine9vn Oct 19, 2023

thijs-franck Oct 19, 2023

janine9vn Oct 19, 2023

janine9vn Oct 19, 2023

		# chose contrast based on the originals image brightness
		watermark_contrast = int(self._get_brightness() * 0.8)

		@@ -0,0 +1,145 @@
		"""Scramble functions for scrambling images in different ways."""

		num_tiles = len(picture.tiles)
		for ring_position in range(1, num_tiles):

Dynamic Typists #2

Are you sure you want to change the base?

Dynamic Typists #2

Conversation

janine9vn commented Sep 28, 2023

janine9vn left a comment

Choose a reason for hiding this comment

Documentation & README

Code Quality & Organization

Commits

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment