Skip to content
This repository has been archived by the owner on Dec 25, 2023. It is now read-only.

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

Merged
merged 50 commits into from
Nov 26, 2022
Merged

Conversation

nagmat84
Copy link
Collaborator

@nagmat84 nagmat84 commented Sep 18, 2022

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".

@nagmat84 nagmat84 changed the title Merge Frontend Modes gallery, view and frame into a Unified Frontend Merge Frontend Modes gallery, view and frame Into a Unified Frontend Sep 19, 2022
@nagmat84 nagmat84 marked this pull request as ready for review September 22, 2022 18:46
@nagmat84 nagmat84 requested a review from a team September 22, 2022 18:46
@nagmat84 nagmat84 force-pushed the merge_modes branch 3 times, most recently from f85d021 to 642bb06 Compare October 19, 2022 12:36
@kamil4
Copy link
Contributor

kamil4 commented Oct 30, 2022

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
Copy link
Collaborator Author

nagmat84 commented Nov 1, 2022

Sooooorry. 😢 (And also sorry for not responding in timely manner.) I made an attempt to fix it a second time in 6e395bc. (I also synced the frontend into the backend LycheeOrg/Lychee#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.

Copy link
Contributor

@kamil4 kamil4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spotted one minor issue during the review. I also still need to test it.

scripts/main/header.js Show resolved Hide resolved
@kamil4
Copy link
Contributor

kamil4 commented Nov 6, 2022

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

  • Uploading to the root album (albums view) doesn't work for an admin for me (weirdly, it seems to pass an ID of one of the tag albums in the Photo:add request?!). Unusually, it works for non-admin users.

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

  • Album owners don't get to see the "Share" button in the header until the album is made public and the "Share button is visible" is checked. This is contrary to the behavior of other options for authenticated album owners (such as the ability to download -- it's always there for owners, irrespective of any settings).
  • When the album is made public and the "Share button is visible" is checked, going (as an authenticated owner) to one of the photos inside the album and opening the visibility dialog is supposed to display read-only visibility status, in this case including a checked "Share button is visible". But that button is not checked.

@nagmat84 nagmat84 mentioned this pull request Nov 6, 2022
@nagmat84
Copy link
Collaborator Author

nagmat84 commented Nov 6, 2022

The other issue should (hopefully) been fixed by #349 on current master. I merged current master into this branch once again.

@kamil4
Copy link
Contributor

kamil4 commented Nov 6, 2022

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
Copy link
Collaborator Author

nagmat84 commented Nov 7, 2022

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.

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. LGTM.

@ildyria ildyria changed the title Merge Frontend Modes gallery, view and frame Into a Unified Frontend Merge gallery, view and frame Into a Unified Frontend Nov 26, 2022
@ildyria ildyria merged commit c2ec2f8 into master Nov 26, 2022
@ildyria ildyria deleted the merge_modes branch November 26, 2022 16:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants