Skip to content

Fixes check for Image Annotation Tool to include ID from image server and fix scrolling bug #5305

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

Merged
sarina merged 3 commits into from
Sep 24, 2014
Merged

Fixes check for Image Annotation Tool to include ID from image server and fix scrolling bug #5305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5634096
Image Annotation Tool: Fixes _isOpenSeaDragon function
lduarte1991 Sep 19, 2014
b7313d6
Annotation Tools: Pylint Fix left over from previous PR
lduarte1991 Sep 19, 2014
29cddbc
Annotation Tools: Non-critical highlights == undefined fixes
lduarte1991 Sep 19, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/lib/xmodule/xmodule/imageannotation_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def __init__(self, *args, **kwargs):
if self.runtime.get_real_user is not None:
try:
self.user_email = self.runtime.get_real_user(self.runtime.anonymous_student_id).email
except: # pylint: disable=broad-except
except Exception: # pylint: disable=broad-except
Copy link
Contributor

@singingwolfboy singingwolfboy Sep 19, 2014

Choose a reason for hiding this comment

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

This is better than it was, but still not great. Can we narrow the scope of what exceptions are being caught? (Same with the other instances of this change in other files.)

(This comment is optional: as I said, this is better than it was. I just wish it could be even better.)

Copy link
Contributor Author

@lduarte1991 lduarte1991 Sep 23, 2014

Choose a reason for hiding this comment

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

@sarina I can't replicate the error anymore, can you doublecheck that "AttributeError" is the appropriate exception for this?

Copy link
Contributor

@sarina sarina Sep 23, 2014

Choose a reason for hiding this comment

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

@lduarte1991 - I'm not sure how to double check this. Personally I'm fine
with "except Exception" - I understand it makes coding nicer, but the fact
is, we don't actually care what the exception here is because what we want
to do is we want to just move forward.

On Tue, Sep 23, 2014 at 1:21 PM, lduarte1991 notifications@github.com
wrote:

In common/lib/xmodule/xmodule/imageannotation_module.py:

@@ -115,7 +115,7 @@ def init(self, _args, *_kwargs):
if self.runtime.get_real_user is not None:
try:
self.user_email = self.runtime.get_real_user(self.runtime.anonymous_student_id).email

  •        except:  # pylint: disable=broad-except

  •        except Exception:  # pylint: disable=broad-except

@sarina https://github.com/sarina I can't replicate the error anymore,
can you doublecheck that "AttributeError" is the appropriate exception for
this?

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/5305/files#r17923634.

self.user_email = _("No email address found.")

def _extract_instructions(self, xmltree):
Expand Down
2 changes: 1 addition & 1 deletion common/lib/xmodule/xmodule/textannotation_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def __init__(self, *args, **kwargs):
if self.runtime.get_real_user is not None:
try:
self.user_email = self.runtime.get_real_user(self.runtime.anonymous_student_id).email
except: # pylint: disable=broad-except
except Exception: # pylint: disable=broad-except
self.user_email = _("No email address found.")

def _extract_instructions(self, xmltree):
Expand Down
2 changes: 1 addition & 1 deletion common/lib/xmodule/xmodule/videoannotation_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def __init__(self, *args, **kwargs):
if self.runtime.get_real_user is not None:
try:
self.user_email = self.runtime.get_real_user(self.runtime.anonymous_student_id).email
except: # pylint: disable=broad-except
except Exception: # pylint: disable=broad-except
self.user_email = _("No email address found.")

def _extract_instructions(self, xmltree):
Expand Down
15 changes: 8 additions & 7 deletions common/static/js/vendor/ova/OpenSeaDragonAnnotation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,13 +577,14 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
var isRP = (typeof rp!='undefined');
var isSource = false;

// Double checks that the image being displayed matches the annotations
var source = this.viewer.source;
var tilesUrl = typeof source.tilesUrl!='undefined'?source.tilesUrl:'';
var functionUrl = typeof source.getTileUrl!='undefined'?source.getTileUrl:'';
var compareUrl = tilesUrl!=''?tilesUrl:('' + functionUrl).replace(/\s+/g, ' ');
if(isContainer) isSource = (an.target.src == compareUrl);

// Though it would be better to store server ids of images in the annotation that
// would require changing annotations that were already made, instead we check if
// the id is a substring of the thumbnail, which according to OpenSeaDragon API should be the case.
var sourceId = this.viewer.source['@id'];
var targetThumb = an.target.thumb;
if (isContainer) {
isSource = (targetThumb.indexOf(sourceId) !== -1);
}
return (isOpenSeaDragon && isContainer && isImage && isRP && isSource);
Copy link
Contributor

@adampalay adampalay Sep 23, 2014

Choose a reason for hiding this comment

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

nit: if isSource is true, then isContainer is true -> we don't need to check for isContainer here

EDIT: no need to fix in this PR; consider for future refactoring

},

Expand Down
10 changes: 7 additions & 3 deletions common/static/js/vendor/ova/catch/js/catch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,9 @@ CatchAnnotation.prototype = {
var an = allannotations[item];
// Makes sure that all images are set to transparent in case one was
// previously selected.
an.highlights[0].style.background = "rgba(0, 0, 0, 0)";
if (an.highlights) {
an.highlights[0].style.background = "rgba(0, 0, 0, 0)";
}
if (typeof an.id !== 'undefined' && an.id === osdaId) { // this is the annotation
var bounds = new OpenSeadragon.Rect(an.bounds.x, an.bounds.y, an.bounds.width, an.bounds.height);
osda.viewer.viewport.fitBounds(bounds, false);
Expand All @@ -883,7 +885,9 @@ CatchAnnotation.prototype = {
'slow');
// signifies a selected annotation once OSD has zoomed in on the
// appropriate area, it turns the background a bit yellow
an.highlights[0].style.background = "rgba(255, 255, 10, 0.2)";
if (an.highlights !== undefined) {
an.highlights[0].style.background = "rgba(255, 255, 10, 0.2)";
}
}
}
},
Expand Down Expand Up @@ -912,7 +916,7 @@ CatchAnnotation.prototype = {
startOffset = hasRanges?an.ranges[0].startOffset:'',
endOffset = hasRanges?an.ranges[0].endOffset:'';

if (typeof startOffset !== 'undefined' && typeof endOffset !== 'undefined') {
if (typeof startOffset !== 'undefined' && typeof endOffset !== 'undefined' && typeof an.highlights) {

$(an.highlights).parent().find('.annotator-hl').removeClass('api');
// change the color
Expand Down