Livewire/more work

[ildyria]

• Repair broken livewire alpha stages.
• add sidebar for albums
• add sidebar for photo
• add login modal.

[codecov[bot]]

Codecov Report

Merging #1303 (331c5b8) into master (6b2b04c) will increase coverage by 2.72%. The diff coverage is 93.38%.

:exclamation: Current head 331c5b8 differs from pull request most recent head 3bffd15. Consider uploading reports for the commit 3bffd15 to get more accurate results

Additional details and impacted files

[nagmat84]

I started a review of this PR and scanned about the first 20 files. Unfortunately, I don't know a polite way how to express it, but I am very unhappy about this PR. This is mostly due to two aspects:

1. Files below the folder Http/Livewire: It took me a lot of work and (tedious) labor to annotate all methods with proper phpDoc type annotations, adding PHP type hints where PHP allows it (for class properties, parameters, return types), etc. The code for the Livewire part misses this to a great deal. On the one hand side, I understand that this is an on-going work and maybe some code will only exist temporarily, in case it turns out that this code is not needed later and so it might be not worth the effort to properly annotate everything. On the other hand side, I fear that we won't re-check and re-review this code again, once it is included in the master branch. I would not like to see this code permanently. May be we can move the code in Http/Livewire into a sub-module for now? But I am reluctant to accept this code for the master branch.
2. Changes to other parts of the code: I understand that Livewire poses certain requirements on classes and objects if those shall be compatible with Livewire, e.g. some classes need to implement the Wireable interface. This is fine for me. However, I don't like that - for example - certain properties of classes just turned from private/protected to public like here https://github.com/LycheeOrg/Lychee/pull/1303/files#diff-685a8b38dd767b54a2c41cb6396510ede81444612e55f7552cf8541158f7ec03. In this particular example, it was not merely a theoretic concern why these attributes are not public, but because adding a size variant to a photo model (or removing it) must be synchronized between the size variant and the photo model otherwise the relation will break. So it is important to call the correct setter/getters and I doubt that the Livewire framework will do so without taking special care of that.

Of course, I can/will have a look into point 2 and try to figure out what it means to make an object "wireable" and and what (possibly undesired or unexpected) side effect this may entail. It is only that I cannot assess the consequences of that right now. From my perspective, it is not as easy as just adding the "wireable" interface and a trait wireable and then hope for the best (not with anything which is based on Laravel). So this won't be a "quick and easy" PR but take some (potentially more) time to review.

[ildyria]

1. No worries, I live in Netherlands, I understand straight feedback.

May be we can move the code in Http/Livewire into a sub-module for now?

Can you explain a bit more ? Do note that Livewire changes app/http/livewire, but also app/view and resources/...

2. The problem comes from the way Livewire serialize and deserialize the data. It actually took me quite a bit of time to debug this part especially since we move to to an infra with AbstractAlbums interface (which makes it a hell for Livewire), the reason being that when rehydrating the model, in the case of an Abstract Property Livewire is not able to know which object to regenerate. By default without the Wireable, it is using toArray() to deserialize and use the construction of a model from an array to rehydrate. This obviously only works if the attributes matches exactly the keys of the array. Due to your refactoring, this is no longer the case, especially given that some are computed on the fly.

[kamil4]

1. No worries, I live in Netherlands, I understand straight feedback.

:rofl: This is soooo true about the Dutch. :rofl:

[nagmat84]

1. No worries, I live in Netherlands, I understand straight feedback.

May be we can move the code in Http/Livewire into a sub-module for now?

Can you explain a bit more ? Do note that Livewire changes app/http/livewire, but also app/view and resources/...

Unfortunately, no. This was more of an unfiltered brain dump, not a well thought out suggestion. I only wanted to spin up some alternative ideas and not ask to improve the code right ahead.

2. The problem comes from the way Livewire serialize and deserialize the data. [...], in the case of an Abstract Property Livewire is not able to know which object to regenerate. By default [...] it is using toArray() [...]. This obviously only works if the attributes match exactly the keys of the array.

Even without looking into the code of Livewire, it was clear that it had to be something like that. But bluntly, this is a technical justification, not a valid argument. It is just again another bad design decision (not yours, but of the Lifewire framework) to assume that every class is standard-constructable from its attributes. However, the "solution" cannot be to just make every attribute public. Maybe, the code seemingly runs (i.e. the PHP framework does not throw a fatal error, because something tries to access a non-public attribute), but this only covers the actual problem and this has to fail when we really start testing the code.

Maybe, I'll find the time to look into the internals of Livewire in the next week or so and see, if I can find a viable solution. But I already fear that we (me?) end up with patching a lot of Livewire and that we will constantly be hitting problems. :(

[ildyria]

However, the "solution" cannot be to just make every attribute public.

It just that the attributes in an API case are invisibly converted in with toJson / toArray upon sending. By adding the wireable methods such as :

	/**
	 * {@inheritdoc}
	 */
	public function toLivewire()
	{
		return $this->id;
	}

	/**
	 * {@inheritdoc}
	 */
	public static function fromLivewire($value)
	{
		$albumFactory = resolve(AlbumFactory::class);

		return $albumFactory->createSmartAlbum($value, true);
	}

We are effectively serializing for livewire only the id an no longer the full object. That way we solve the problem of toArray() serialization.

One of the advantage of Livewire is to be able to be able to apply php logic on the server side rather on the client side. This is quite convenient when dealing with access rights, but also applying modifications on a Model: you can just update the model, save() and the rendering is automated. Furthermore you get more explicit errors than the one we had in JS.

In this particular example, it was not merely a theoretic concern why these attributes are not public, but because adding a size variant to a photo model (or removing it) must be synchronized between the size variant and the photo model otherwise the relation will break. So it is important to call the correct setter/getters and I doubt that the Livewire framework will do so without taking special care of that.

In the case of Thumb there is not much difference between doing a toArray() and accessing the attributes directly. But I do agree that accessing relations should be done carefully.

Also important notion. In the Livewire objects (components) and view objects, only public attributes are accessible by the blade template.

[nagmat84]

One of the advantage of Livewire is to be able to be able to apply php logic on the server side rather on the client side. This is quite convenient when dealing with access rights, but also applying modifications on a Model: you can just update the model, save() and the rendering is automated. Furthermore you get more explicit errors than the one we had in JS.

I haven't doubt that, but that's not the point.

And by the way, the problem that we had no proper error message in the JS frontend was entirely home brewed, because it was decided to ignore proper exception handling and hiding a failing request behind an unspecific false. This should be solved now.

But I do agree that accessing relations should be done carefully.

Fine. In particular, SiveVariants can be thought of a a highly specialized replacement for a generic Collection of a HasMany-relation and hence must be treated like one.

We have a (1:7)-relation between a Photo model and a SizeVariant model (note the singular). A usual HasMany-relation would return a Collection<SizeVariant>, but such a collection neither respects that there can only at most on SizeVariant per photo of each type not does it allow to access a specific type of size variant directly. If one wanted to access a specific type inside a generic Collection, one would have to iterate through the collection every time.

Keeping in mind that SizeVariants plays the same role for HasManySizeVariants ad Collection does for HasMany, I conjuncture that it should not be necessary to make SizeVariants Wireable ore expose its internals. (It should be treated like a relation collection.) Violating this just smells wrong.

Also important notion. In the Livewire objects (components) and view objects, only public attributes are accessible by the blade template.

Sounds like another crazy design decision/assumption. But is is not entirely true, see here: https://laravel-livewire.com/docs/2.x/properties#computed-properties

[ildyria]

Sounds like another crazy design decision/assumption. But is is not entirely true, see here: laravel-livewire.com/docs/2.x/properties#computed-properties

Yes, I know, I am already using this at some places. :)

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

No Coverage information
0.0% Duplication

[ildyria]

Requires: #1785

[Fensterbank]

It's nice seeing a lot of work happening here. 👏 :tada: Will this new frontend also fix the fact, that the current site cannot be navigated by search engines? This would mean using proper links instead of <div> with onClick event and getting rid of the #hash url approach. The page could still be feel like a single-site-application, most modern frameworks use a fallback approach using JavaScript for modern browsers while server side navigation always works as a fallback.

This would make #1646 less important since search engines would just navigate the page for themselves.

[ildyria]

It's nice seeing a lot of work happening here. 👏 🎉

Yes, however if you look at the creation date of the pull request, you can see that I have been working on this since May 2022, so nearly 1 year ago... I am dedicating to LycheeOrg all the free time I have left after Sports (mens sana in corpore sano) and the Lady. :sweat_smile: So I would not expect this to be released before minimum another month if not more...

Will this new frontend also fix the fact, that the current site cannot be navigated by search engines? This would mean using proper links instead of

with onClick event and getting rid of the #hash url approach.

Yes. :)

It will also improve the development of pages. So implementing a Site map should be easier.

[ildyria]

Merging #2012 will be first step to :heavy_check_mark: #383 Second step is obviously checking the rights in the Livewire side.

[qwerty287]

So a few points I noticed just now (maybe I'd copy this again once we have a new PR):

1. Using the "back" button is extremely slow. It takes ~2 sec to load. Opening a new view (e.g. album) is fast though.
2. The layout selector is also shown if there are no photos, I think it would better to hide it.
3. Dialogs and right-click menus are still extremely slow.
4. Navigating left/right on photos is extremely slow, it would be better to navigate directly and show a loading indicator.
5. If you open/close the photo sidebar very often, at some point it does not work anymore and is always hidden.
6. Opening the photo sidebar the first time moves the photo left first, then nothing happens for a short time, and then the sidebar opens. This should be synchronously.
7. I'm not a big fan of the big buttons for photos, that gives me the feel this is software for children ;)
8. When moving photos, the album list loads very long (it's pretty fast when moving albums though).
9. Opening the album detail panel does not animate 100% fluently.
10. Text inputs in dialogs have a weird border (e.g. login)

Overall it's working well (except the performance as I wrote above...), thanks for all the work!

[ildyria]

Squashed & closed. See https://github.com/LycheeOrg/Lychee/pull/2035 for the updated PR.