search

LycheeOrg / Lychee-front

JS implementation of Lychee frontend
https://lycheeorg.github.io/
MIT License
48 stars 53 forks source link

Merge `gallery`, `view` and `frame` Into a Unified Frontend #323

Closed nagmat84 closed 1 year ago

nagmat84 commented 2 years ago

This PR merges the three distinct frontend modes "gallery", "view" and "frame" into a single unified framework. In doing so, many code duplications and "sub implementations" of JS objects are thrown out.

This PR has taken the different Blade views of the backend and merged the resulting HTML markup into a new HTML file frontend.html.

The view mode is now accessible via #view/<photo-id>, the frame mode via `#frame' . Redirections from the old URLs are enabled via the backend.

A note and disclaimer

I am not very fond of this PR myself either. Basically, it is an intermediate step with many things left to do. In particular, the new "root" HTML file frontend.html puts the three different modes as independent <div> elements one after another and CSS ensures that only one of these elements is visible at a time. This is not my ultimate goal.

Independent of the current security issue I developed some ideas how to re-structure the whole frontend and improve on its (currently non-existing) modular design. In order to not accidentally break things for the view and frame mode, I wanted to take those into account right from the beginning. This implied that they needed to be merged first.

In order to avoid a single "big bang" PR which takes years to review, I wanted to get this merge out of the way with the least amount of lines of newly added code as possible and with the least amount of change as possible. Unfortunately, this also means that there had to be some nasty workarounds.

Addendum

The failing CI is caused by a moved file which has otherwise not been changed by this PR. The CI complains about a poor font definition which the CI considers to be "new".

kamil4 commented 2 years ago

Something's definitely broken with the header buttons in the photo view in public mode. I don't see the full screen button, the more menu (with the download button)... On the other hand, the share button seems to be always there even if it shouldn't...

nagmat84 commented 2 years ago

Sooooorry. :cry: (And also sorry for not responding in timely manner.) I made an attempt to fix it a second time in https://github.com/LycheeOrg/Lychee-front/pull/323/commits/6e395bc6400a0f2c951410e820d118397a2235c3. (I also synced the frontend into the backend https://github.com/LycheeOrg/Lychee/pull/1522, hence you should probably checkout that.)

I tested both ways to display a photo, i.e. in gallery mode via #<album-id>/<photo-id> and in view mode via /view?p=<photo-id> for anonymous and admin user. I assume we should be fine now.

Note, in contrast to the current master branch you should now see some more buttons in "view mode" if a user is logged in. On current master, the "view mode" only shows the share button and the info button. On this branch, the "view mode" shows the same buttons as the gallery mode does for an unauthenticated user, i.e. the share button, the info button, the map button (if enabled), the fullscreen button (if fullscreen is available) and the more button (if not empty, i.e. if downloading the original size variant is enabled). However, you should only see button which are "safe", i.e. which don't throw an exception if used.

I had to fix the method album.isUploadable which was the culprit all the way along. I hope this did not break anything else, but I haven't found anything so far.

kamil4 commented 2 years ago

I played with it a bit this evening and unfortunately this is still not ready for merging due to the following problem:

While testing, I also noticed some strange behavior around the "Share" button although it looks like that may not be specific to this branch:

nagmat84 commented 2 years ago

The other issue should (hopefully) been fixed by https://github.com/LycheeOrg/Lychee-front/pull/349 on current master. I merged current master into this branch once again.

kamil4 commented 2 years ago

The share button became visible for me, but the dialog box still wouldn't open. I removed the checks from the code, which I believe were unnecessary since the proper access control is done at the button visibility level. Sorry for polluting your PR with this code but since the discussion started here...

I also figured out my issue with being unable to upload to root. It was due to the relative positions of my browser window (and its contents) and the file selection dialog. My mouse pointer happened to be hovering over a tag album at the time and if that's the case, album.getID returns the ID of that album, not null!!! This is a frightening but undoubtedly old bug so I guess we should let it be for now...

nagmat84 commented 2 years ago

The share button became visible for me, but the dialog box still wouldn't open. I removed the checks from the code, which I believe were unnecessary since the proper access control is done at the button visibility level. Sorry for polluting your PR with this code but since the discussion started here...

No worries. I'm glad that you checked the PR that thoroughly. It is simply interesting how many bugs suddenly become apparent which probably have been lurking around for quite some while.

No worries about "polluting" this PR. I think the code (not necessarily this PR, but the frontend in general) has reached some that state such that it is hardly possible to pollute it any more. 😂 The whole visibility checks are completely crazy.

I also figured out my issue with being unable to upload to root. It was due to the relative positions of my browser window (and its contents) and the file selection dialog. My mouse pointer happened to be hovering over a tag album at the time and if that's the case, album.getID returns the ID of that album, not null!!! This is a frightening but undoubtedly old bug so I guess we should let it be for now...

Thats fits in the overall picture. But this is also an indicator that my recently added comment to the code where the "drop" event handler is registered is more than justified. It is just insane to register the "drop" event handler on the global browser window and then attempt to find out which element really received the drop element. This has to fail. However, this can only be fixed in the long run and only after the box model has been revamped.