Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add width, height, crop and mode to BeforePreviewFetchedEvent #38679

Merged
merged 1 commit into from
Jun 26, 2023
Merged

feat: add width, height, crop and mode to BeforePreviewFetchedEvent #38679

merged 1 commit into from
Jun 26, 2023

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 6, 2023 •
edited
Loading

Summary

user_usage_report keeps a counter for files read.1

The app uses

  • OC_Filesystem::read hook
  • Event listener (the legacy one) for IPreview::EVENT

Request for previews do not trigger the hook, hence the additional event listener.
The thumbnails for a list or grid view should not count, so we need the width and height.

TODO

  • CI

Checklist

Footnotes

  1. https://github.com/nextcloud/user_usage_report/blob/c12e97d84033818af27ae5cf659c88f743137be7/lib/AppInfo/Application.php#L51 ↩

@kesselb kesselb added the 3. to review Waiting for reviews label Jun 6, 2023
@kesselb kesselb self-assigned this Jun 6, 2023
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jun 7, 2023
ChristophWurst
ChristophWurst requested changes Jun 7, 2023
View reviewed changes
lib/public/Preview/BeforePreviewFetchedEvent.php Outdated
Comment on lines 40 to 41
private int $width,
private int $height) {
Copy link
Member

Choose a reason for hiding this comment

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

The two new required constructor arguments form a breaking change for apps creating an instance, e.g. to dispatch their own event.

Can we assume that apps don't do that? Then the change could be ok.

Otherwise I would prefer to make the two new properties optional and nullable, document that in three versions (nextcloud 31) the arguments are required and only then make them non-null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we assume that apps don't do that? Then the change could be ok.

I assume that ;)

@tcitworld
Copy link
Member

Would you mind adding more details while we're at it? What I added in #37193 for admin_audit (crop, mode and mimetype).

@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch from a91d343 to 137a4e8 Compare June 12, 2023 09:46
@kesselb kesselb changed the title feat: add width and height to BeforePreviewFetchedEvent feat: add width, height, crop and mode to BeforePreviewFetchedEvent Jun 12, 2023
@kesselb
Copy link
Contributor Author

kesselb commented Jun 12, 2023

@tcitworld added crop and mode.

@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch 2 times, most recently from 6a0311b to aaf6a3e Compare June 13, 2023 17:11
@kesselb kesselb added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 20, 2023
@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch from aaf6a3e to bf2346a Compare June 20, 2023 08:59
@kesselb
Copy link
Contributor Author

kesselb commented Jun 20, 2023

Added defaults for width, height, crop and mode to keep the public api stable.

@kesselb kesselb added this to the Nextcloud 28 milestone Jun 20, 2023
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 20, 2023
@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch from bf2346a to 4f0e351 Compare June 22, 2023 16:35
ChristophWurst
ChristophWurst reviewed Jun 23, 2023
View reviewed changes
lib/public/Preview/BeforePreviewFetchedEvent.php Outdated
public function __construct(
private Node $node,
/** @deprecated 28.0.0 default value is deprecated **/
private int $width = 0,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's better to have wrong data (0x0px image) or null values when the emitter doesn't pass values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True True

@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch from 4f0e351 to adaf879 Compare June 23, 2023 12:51
ChristophWurst
ChristophWurst approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Psalm sugar

lib/public/Preview/BeforePreviewFetchedEvent.php Outdated
Comment on lines 80 to 85
/**
* @since 28.0.0
*/
public function getMode(): ?string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* @since 28.0.0
*/
public function getMode(): ?string {
/**
* @since 28.0.0
* @return null|IPreview::MODE_*
*/
public function getMode(): ?string {

lib/public/Preview/BeforePreviewFetchedEvent.php
@@ -32,14 +32,21 @@
* @since 25.0.1
*/
class BeforePreviewFetchedEvent extends \OCP\EventDispatcher\Event {
private Node $node;

/**
* @since 25.0.1
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*/
* @param string|IPreview::MODE_*
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

@kesselb
feat: add width, height, crop and mode to BeforePreviewFetchedEvent
440f882 
user_usage_report keeps a counter for files read.

The app uses

- OC_Filesystem::read hook
- Event listener (the legacy one) for IPreview::EVENT

Request for previews do not trigger the hook, hence the additional event listener.
The thumbnails for a list or grid view should not count, so we need the width and height.

Signed-off-by: Daniel Kesselberg <[email protected]>
@kesselb kesselb force-pushed the feat/add-spec-to-preview-fetched-event branch from adaf879 to 440f882 Compare June 23, 2023 14:27
@kesselb kesselb mentioned this pull request Jun 26, 2023
Merged
@kesselb
Copy link
Contributor Author

kesselb commented Jun 26, 2023

Documentation: nextcloud/documentation#10674

@kesselb kesselb merged commit 9751303 into master Jun 26, 2023
38 checks passed
@kesselb kesselb deleted the feat/add-spec-to-preview-fetched-event branch June 26, 2023 11:53
@kesselb kesselb removed the pending documentation This pull request needs an associated documentation update label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@st3iny st3iny st3iny approved these changes

@tcitworld tcitworld tcitworld approved these changes

Assignees

@kesselb kesselb

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
4 participants
@kesselb @tcitworld @ChristophWurst @st3iny