Skip to content

[Editor] Add a new dialog for alt-text settings (bug 1909604)#18537

Merged
calixteman merged 1 commit into
mozilla:masterfrom
calixteman:image_settings
Aug 1, 2024
Merged

[Editor] Add a new dialog for alt-text settings (bug 1909604)#18537
calixteman merged 1 commit into
mozilla:masterfrom
calixteman:image_settings

Conversation

@calixteman

@calixteman calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor

This patch adds a new entry in the secondary menu in order to open a dialog to let the user:

  • disables the alt-text generation thanks to a ML model;
  • deletes the alt-text model downloaded in Firefox;
  • disabled the new alt-text flow.

@calixteman calixteman requested a review from a team as a code owner August 1, 2024 13:48
@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

/botio-linux preview

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/d3b4a5c8530789e/output.txt

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/d3b4a5c8530789e/output.txt

Total script time: 1.04 mins

Published

flodolo
flodolo reviewed Aug 1, 2024
View reviewed changes
Comment thread l10n/en-US/viewer.ftl
@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

@Snuffleupagus, could you review it asap, today ideally, at least to catch obvious mistakes, and I'll do some follow-ups to fix less obvious errors ?
We're in soft-freeze and I'd like to make a release today. I'll uplift follow-ups (if any) and the telemetry next week or tomorrow.

@Snuffleupagus

Snuffleupagus commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

could you review it asap, today ideally, at least to catch obvious mistakes, and I'll do some follow-ups to fix less obvious errors ?

Well, the preview above doesn't even load correctly :-P

Uncaught (in promise) TypeError: mlManager is null
    ImageAltTextSettings new_alt_text_manager.js:497
    _initializeViewerComponents app.js:561
    initialize app.js:253
new_alt_text_manager.js:497:36

@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

could you review it asap, today ideally, at least to catch obvious mistakes, and I'll do some follow-ups to fix less obvious errors ?

Well, the preview above doesn't even load correctly :-P

Uncaught (in promise) TypeError: mlManager is null
    ImageAltTextSettings new_alt_text_manager.js:497
    _initializeViewerComponents app.js:561
    initialize app.js:253
new_alt_text_manager.js:497:36

arf.. yes it's because we've a GENERIC build.

@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

/botio-linux preview

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/535445e9032f13e/output.txt

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/535445e9032f13e/output.txt

Total script time: 1.03 mins

Published

Snuffleupagus
Snuffleupagus requested changes Aug 1, 2024
View reviewed changes

@Snuffleupagus Snuffleupagus left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unfortunately some of this new functionality isn't all that easy to follow (at least not to me), so I'm slightly worried about the maintainability of all of this new code.
Given the apparent time pressure in getting this landed, it does seem to me that all of this could probably benefit from some future re-factoring and clean-up to simplify things?

Comment thread src/display/editor/tools.js Outdated
Comment thread src/display/editor/tools.js Outdated
Comment thread web/annotation_editor_layer_builder.css
Comment thread web/app.js Outdated
Comment thread web/app.js Outdated
Comment thread web/new_alt_text_manager.js Outdated
Comment thread web/new_alt_text_manager.js
Comment thread web/new_alt_text_manager.js Outdated
Comment thread web/new_alt_text_manager.js Outdated
Comment thread web/stubs-geckoview.js
@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

Unfortunately some of this new functionality isn't all that easy to follow (at least not to me), so I'm slightly worried about the maintainability of all of this new code. Given the apparent time pressure in getting this landed, it does seem to me that all of this could probably benefit from some future re-factoring and clean-up to simplify things?

Yes for sure, I hope to improve this when I'll write the integration tests for the new feature.

@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

/botio-linux preview

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/05a20ba461541af/output.txt

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/05a20ba461541af/output.txt

Total script time: 1.07 mins

Published

Snuffleupagus
Snuffleupagus approved these changes Aug 1, 2024
View reviewed changes

@Snuffleupagus Snuffleupagus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

r=me, with a couple of final comments; thank you!

Comment thread src/display/editor/tools.js Outdated
Comment thread web/app_options.js Outdated
Comment thread web/new_alt_text_manager.js Outdated
timvandermeij
timvandermeij reviewed Aug 1, 2024
View reviewed changes
Comment thread web/app.js Outdated
@calixteman
[Editor] Add a new dialog for alt-text settings (bug 1909604)
32d0927 
This patch adds a new entry in the secondary menu in order to open a dialog to let the user:
 - disables the alt-text generation thanks to a ML model;
 - deletes the alt-text model downloaded in Firefox;
 - disabled the new alt-text flow.
@calixteman

calixteman commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

/botio integrationtest

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/68074a25244b31a/output.txt

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/89ecb0a44448415/output.txt

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/68074a25244b31a/output.txt

Total script time: 8.50 mins

  • Integration Tests: Passed

@moz-tools-bot

moz-tools-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/89ecb0a44448415/output.txt

Total script time: 17.94 mins

  • Integration Tests: FAILED

@calixteman calixteman merged commit 501da85 into mozilla:master Aug 1, 2024
@calixteman calixteman deleted the image_settings branch August 1, 2024 19:55
@YuvrajKaushal YuvrajKaushal mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timvandermeij timvandermeij timvandermeij left review comments
@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes
@flodolo flodolo flodolo approved these changes

Assignees

No one assigned

Labels

editor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@calixteman @moz-tools-bot @Snuffleupagus @timvandermeij @flodolo