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.

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

Add global setting for sent media quality #5781

Closed
wants to merge 1 commit into from

Conversation

urandom2
Copy link

@urandom2 urandom2 commented Feb 4, 2022

First time contributor checklist:

Contributor checklist:

Description

Following in the shoes of Android and iOS, this change adds a global
setting for defining a preference for standard or high quality media
uploads in conversations.

This work takes inspiration from the theme and spell check settings that
are structurally similar (drop down select) and nearby in the ui (also
in Chats), respectively. Some patterns may be incorrect or dated, as
these were selected not based on how recently they were added.

No new features were added that needed novel testing pathways, we simply
wired up an existing boolean with a global default. As such,
functionality has been manually verified on a linux, and confirmed to
set the global default as desired. Some changes were also made to
Preferences.stories.tsx so that it would compile.

Screenshot

Not much to see here, as it still looks the same, but you can now get high quality by default:
2022-02-04-231545_960x1063_scrot

Definitely went the easy route and simply added a new element, without a new section or description text; also the alignment with the checkboxes means it may not look super pretty, but it does what it needs to:
2022-02-04-231451_700x700_scrot

@josh-signal
Copy link
Contributor

Can we get a screenshot of what it looks like?

@@ -547,6 +547,9 @@ export class ConversationView extends window.Backbone.View<ConversationModel> {
// setupCompositionArea
window.reduxActions.composer.resetComposer();

window.reduxStore.getState().composer.shouldSendHighQualityAttachments =
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't just overwrite state in redux like this.

The global setting state should already live in the items duck so how about we create a selector that derives the state?

Copy link
Author

Choose a reason for hiding this comment

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

whew, such a relief; this was far too much a hack for my taste, but I was poking around for the fuse in the dark; thanks a bunch for the ducks reference, would have taken me way too long to find it.

Copy link
Contributor

@josh-signal josh-signal left a comment

Choose a reason for hiding this comment

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

Good, but the state handling needs to be fixed.

Following in the shoes of Android and iOS, this change adds a global
setting for defining a preference for standard or high quality media
uploads in conversations.

This work takes inspiration from the theme and spell check settings that
are structurally similar (drop down select) and nearby in the ui (also
in Chats), respectively. Some patterns may be incorrect or dated, as
these were selected not based on how recently they were added.

No new features were added that needed novel testing pathways, we simply
wired up an existing boolean with a global default. As such,
functionality has been manually verified on a linux, and confirmed to
set the global default as desired. Some changes were also made to
Preferences.stories.tsx so that it would compile.
@urandom2
Copy link
Author

urandom2 commented Feb 4, 2022

unrelated, this seems like a setting that would be really nice to have live in the user profile alongside settings__DisappearingMessages__timer__label, but that is well outside my wheelhouse.

@urandom2
Copy link
Author

urandom2 commented Mar 3, 2022

@josh-signal: anything else you need from me, or is this just stuck in the review queue?

Copy link
Contributor

@josh-signal josh-signal left a comment

Choose a reason for hiding this comment

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

@arnottcr sorry it took me so long to pull it down and test it. It doesn't seem to be working at the moment because when the REPLACE_ATTACHMENTS action runs it resets the quality to "standard".

I think we'll need 3 different states here (unset, set to standard, set to high) if it's unset we go with whatever your preference is. Feel free to rename the shouldSendHighQualityAttachments property and perhaps use undefined, standard, and high as the states.

@furai
Copy link

furai commented May 16, 2022

Sorry for bumping but what's the status on this? I'd love to see this feature implemented.

@urandom2
Copy link
Author

I gave it a "college try", but as a not frontend dev, this quickly became out of my depth, and I think the core team should probably take the lead on such a refactor. If somebody wants to take over the work, I am happy to handoff.

@josh-signal
Copy link
Contributor

@arnottcr I can take it to the finish line sometime soon. Thanks for the initial work!

@stale
Copy link

stale bot commented Aug 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Nov 29, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 29, 2022
@josh-signal josh-signal removed the stale label Nov 29, 2022
@urandom2
Copy link
Author

yeah, I do not have the bandwidth or background to drive this forward; I am sad to see this feature gap between mobile and desktop, but it is understandable with the signal team size; hope this gets resolved eventually!

@urandom2 urandom2 closed this Nov 29, 2022
@josh-signal
Copy link
Contributor

FYI we merged this in main: 1109415

@urandom2 urandom2 deleted the mediaquality branch January 4, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants