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

[WIP] Add frontend for text based needles #5165

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[WIP] Add frontend for text based needles #5165

wants to merge 4 commits into from

Conversation

clanig
Copy link
Contributor

@clanig clanig commented May 30, 2023

This PR is for the frontend integration of the OCR feature.
It depends on os-autoinst/os-autoinst#2276 to be merged.

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching docs/*.asciidoc:

  • Consider generating documentation locally to verify it is rendered correctly using tools/generate-htmldoc

This is an automatically generated QA checklist based on modified files.

@kalikiana kalikiana marked this pull request as draft May 30, 2023 08:07
Martchus
Martchus requested changes May 30, 2023
View reviewed changes
assets/javascripts/needlediff.js Outdated
this.ctx.drawImage(this.needleImg, orig.xpos, orig.ypos, usedWith, a.height, x, a.ypos, usedWith, a.height);
if (this.needleImg) {
this.ctx.drawImage(this.needleImg, orig.xpos, orig.ypos, usedWith, a.height, x, a.ypos, usedWith, a.height);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole else-block should better be extracted into a separate function (to avoid this one from becoming to big).

assets/javascripts/needlediff.js Outdated
}

// Write OCR lines to Canvas context line by line
var ocrTxtXPos = Math.ceil(a.xpos + a.width / 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would generally be nice to use const/let in new code, e.g.

Suggested change
var ocrTxtXPos = Math.ceil(a.xpos + a.width / 2);
const ocrTxtXPos = Math.ceil(a.xpos + a.width / 2);

This counts also for all the other occurences of var … (in new code).

assets/javascripts/needlediff.js Outdated
} else {
// Calculate font size with 1px margin
var ocrLinesFontSize = 14;
var ocrLines = a['ocr_str'].split('\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var ocrLines = a['ocr_str'].split('\n');
var ocrLines = a.ocr_str.split('\n');

assets/javascripts/needleeditor.js Outdated
var changeMarginButton = document.getElementById('change-margin');
// Only OCR areas can be assigned a refstr
var refstrTxtArea = document.getElementById('txtarea-refstr');
if (a.type == "match") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (a.type == "match") {
if (a.type === "match") {

assets/javascripts/needleeditor.js Outdated
refstrTxtArea.value = '';
refstrTxtArea.classList.add('disabled');
refstrTxtArea.disabled = "disabled";
} else if (a.type == "ocr") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (a.type == "ocr") {
} else if (a.type === "ocr") {

lib/OpenQA/Task/Needle/Save.pm Outdated
my $error = "Image $imagename could not be found!";
$app->log->error("Failed to save needle: $error");
return $minion_job->fail({error => "<strong>Failed to save $needlename.</strong><br>$error"});
unless ($no_match_areas) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The double-negative is not very nice and we won't need to extra variable here.

Suggested change
unless ($no_match_areas) {
if (@match_areas) {

The same counts for the other unless ($no_match_areas) occurrences.

lib/OpenQA/WebAPI/Controller/Step.pm Outdated
@@ -154,7 +154,7 @@ sub edit ($self) {
width => int $match->{w},
ypos => int $match->{y},
height => int $match->{h},
type => 'match',
type => 'match',
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks unintended. Likely tidy will fix it for you (which you'll have to run anyways).

lib/OpenQA/WebAPI/Controller/Step.pm Outdated
@@ -449,6 +450,8 @@ sub calc_matches ($needle, $areas) {
type => $area->{result},
similarity => int($area->{similarity} + 0.5),
);
$match{ocr_str} = $area->{ocr_str} if defined $area->{ocr_str};
print($area->{ocr_str}) if defined $area->{ocr_str};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely for local debugging and should be removed before merging.

docs/Contributing.asciidoc Outdated
@@ -134,8 +134,7 @@ Installation Guide.
In the case of os-autoinst, only a few http://www.cpan.org/[CPAN] modules are
required. Basically `Carp::Always`, `Data::Dump`. `JSON` and `YAML`. On the other
hand, several external tools are needed including
http://wiki.qemu.org/Main_Page[QEMU],
https://code.google.com/p/tesseract-ocr/[Tesseract] and
http://wiki.qemu.org/Main_Page[QEMU] and
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should mention GOCR then instead?

assets/javascripts/needlediff.js Outdated
Comment on lines 142 to 144
while ((ocrLines.length * ocrLinesFontSize + (ocrLines.length - 1)) > (a.height - 2)) {
ocrLinesFontSize--;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be computed via a loop?

@clanig
Display OCR result of test on step screenshot
d20ed4e
@clanig
Implement OCR in needle editor
882d8b1
@clanig
Update docs for OCR implementation
c8103e1
@clanig
Fix character encoding for OCR
3a4e3e1 
The addition of the OCR feature revealed character encoding bugs that
might have been invisible previously. This commit fixes the character
encoding in multiple places to ensure the correct displaying of special
characters in the web frontend.
@clanig clanig force-pushed the add_frontend_for_text_based_needles branch from 0f737df to 3a4e3e1 Compare September 14, 2023 15:00
@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2023

This pull request is now in conflicts. Could you fix it? 🙏

@clanig
Copy link
Contributor Author

clanig commented Sep 14, 2023

I have updated the code. There were multiple encoding bugs as well.
This code is based on an old codebase. So I am going to rebase now.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 6, 2024

This pull request is now in conflicts. Could you fix it? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalikiana kalikiana kalikiana left review comments

@Martchus Martchus Martchus requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clanig @kalikiana @Martchus