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

Milestone 1.3.1: Type definitions for guppy, removed blobbuilder #9278

Merged
merged 9 commits into from
May 11, 2020
Merged

Milestone 1.3.1: Type definitions for guppy, removed blobbuilder #9278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c98b113
Type definitions for blobbuilder, guppy
nishantwrp May 8, 2020
89e1634
Merge remote-tracking branch 'upstream/develop' into blobbuilder-guppy
nishantwrp May 9, 2020
965d9bc
Removed BlobBuilder
nishantwrp May 9, 2020
0c98038
Changed Comment
nishantwrp May 9, 2020
3d8ba96
Lint
nishantwrp May 9, 2020
de5d02a
Definitions for current version
nishantwrp May 9, 2020
071baf7
Definitions for current version
nishantwrp May 9, 2020
0cb20ea
Merge develop
nishantwrp May 10, 2020
2f8195b
Namespace
nishantwrp May 10, 2020
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
79 changes: 0 additions & 79 deletions core/templates/services/assets-backend-api.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ describe('Assets Backend API Service', function() {
exploration_id: '0',
filename: 'myfile.png'
});

window.BlobBuilder = undefined;
}));

afterEach(function() {
Expand Down Expand Up @@ -194,83 +192,6 @@ describe('Assets Backend API Service', function() {
$httpBackend.verifyNoOutstandingExpectation();
});

it('should handler rejection on trying to process fetched file when' +
' Blob throws a TypeError and BlobBuilder does not exist', function() {
var successHandler = jasmine.createSpy('success');
var failHandler = jasmine.createSpy('fail');

spyOn(window, 'Blob').and.callFake(function() {
throw new TypeError();
});

$httpBackend.expect('GET', audioRequestUrl).respond(201, 'Error');
expect(AssetsBackendApiService.isCached('myfile.mp3')).toBe(false);

AssetsBackendApiService.loadAudio('0', 'myfile.mp3').then(
successHandler, failHandler);
$httpBackend.flush();
expect(successHandler).not.toHaveBeenCalled();
expect(failHandler).toHaveBeenCalledWith('myfile.mp3');
$httpBackend.verifyNoOutstandingExpectation();
});

it('should handler rejection on trying to process fetched file when Blob' +
' throws a TypeError and BlobBuilder does not work correctly',
function() {
var successHandler = jasmine.createSpy('success');
var failHandler = jasmine.createSpy('fail');

spyOn(window, 'Blob').and.callFake(function() {
throw new TypeError();
});

window.BlobBuilder = function BlobBuilder() {
this.blob = new Object({ blobBuilder: true });
};

$httpBackend.expect('GET', audioRequestUrl).respond(201, 'Error');
expect(AssetsBackendApiService.isCached('myfile.mp3')).toBe(false);

AssetsBackendApiService.loadAudio('0', 'myfile.mp3').then(
successHandler, failHandler);
$httpBackend.flush();
expect(successHandler).not.toHaveBeenCalled();
expect(failHandler).toHaveBeenCalledWith('myfile.mp3');
$httpBackend.verifyNoOutstandingExpectation();
});

it('should successfully process a fetch file when Blob throws a TypeError' +
Copy link
Member

Choose a reason for hiding this comment

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

Make sure we have another test for the success process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only removed the part where BlobBuilder was involved.

Copy link
Member

@DubeySandeep DubeySandeep May 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please check whether we still have a test that checks the successful creation of Blob out of fetched audio/images data? Maybe share the link to that test block*

[This comment wasn't published earlier]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here

oppia/core/templates/services/assets-backend-api.service.spec.ts

Line 132 in 2f8195b

it('should successfully fetch and cache audio', function() {

' and BlobBuilder is correctly implemented', function() {
var successHandler = jasmine.createSpy('success');
var failHandler = jasmine.createSpy('fail');

spyOn(window, 'Blob').and.callFake(function() {
throw new TypeError();
});

window.BlobBuilder = function BlobBuilder() {
this.blob = new Object({ blobBuilder: true });
this.append = function append(data) {};
this.getBlob = function getBlob(contentType) {
return this.blob;
};
};

$httpBackend.expect('GET', audioRequestUrl).respond(201, 'Error');
expect(AssetsBackendApiService.isCached('myfile.mp3')).toBe(false);

AssetsBackendApiService.loadAudio('0', 'myfile.mp3').then(
successHandler, failHandler);
$httpBackend.flush();
expect(successHandler).toHaveBeenCalledWith(
audioFileObjectFactory.createNew(
'myfile.mp3',
{ blobBuilder: true }
));
expect(failHandler).not.toHaveBeenCalled();
$httpBackend.verifyNoOutstandingExpectation();
});

it('should successfully save an audio', function(done) {
var successMessage = 'Audio was successfully saved.';
// @ts-ignore in order to ignore JQuery properties that should
Expand Down
22 changes: 1 addition & 21 deletions core/templates/services/assets-backend-api.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,27 +91,7 @@ angular.module('oppia').factory('AssetsBackendApiService', [
}).then(function(response) {
var assetBlob = null;
var data = response.data;
try {
assetBlob = new Blob([data], {type: data.type});
} catch (exception) {
window.BlobBuilder = window.BlobBuilder ||
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting these? [This helps us to have this functionality in cross-browser/devices.]

Copy link
Member

Choose a reason for hiding this comment

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

You can use git blame to check when and why this was introduced*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to @vojtechjelinek all the browsers have this new Blob() now.

Copy link
Member

Choose a reason for hiding this comment

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

all the new updated/browsers, right? (Have you check why this was added in the first place? Also, might worth checking server log errors for the errors dict reported through this function)

Copy link
Member

Choose a reason for hiding this comment

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

#9278 (comment)

@kevintab95 Can you please check the server logs for the message that we are deleting?

Copy link
Member

@kevintab95 kevintab95 May 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Per my comment on a different PR, try-catch was added to investigate #6162 and that hasn't occured in a while. It seems to be fixed by #7784 where we added a second argument to the Blob constructor. So as long as we use Blob with the second argument, it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the info, @kevintab95!

window.WebKitBlobBuilder ||
window.MozBlobBuilder ||
window.MSBlobBuilder;
if (exception.name === 'TypeError' && window.BlobBuilder) {
var blobBuilder = new BlobBuilder();
blobBuilder.append(data);
assetBlob = blobBuilder.getBlob(assetType.concat('/*'));
} else {
var additionalInfo = (
'\nBlob construction error debug logs:' +
'\nAsset type: ' + assetType +
'\nData: ' + data
);
exception.message += additionalInfo;
throw exception;
}
}
assetBlob = new Blob([data], {type: data.type});
assetsCache[filename] = assetBlob;
if (assetType === ASSET_TYPE_AUDIO) {
successCallback(
Expand Down
2 changes: 1 addition & 1 deletion ...tions/MathExpressionInput/directives/oppia-interactive-math-expression-input.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ angular.module('oppia').directive('oppiaInteractiveMathExpressionInput', [
DebouncerService, DeviceInfoService, WindowDimensionsService,
CurrentInteractionService) {
var ctrl = this;
var guppyDivElt, guppyDivId, guppyInstance;
var guppyDivElt, guppyDivId, guppyInstance: Guppy;
var oppiaSymbolsUrl = UrlInterpolationService.getStaticAssetUrl(
'/overrides/guppy/oppia_symbols.json');
var labelForFocusTarget = $attrs.labelForFocusTarget || null;
Expand Down
4 changes: 0 additions & 4 deletions typings/custom-window-defs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
// Any property defined on window needs to be added here if is not
// present on the type of window.
interface Window {
BlobBuilder?: any;
CodeMirror?: any;
Date?: any;
GLOBALS?: any;
HTMLElement?: any;
MSBlobBuilder?: any;
Math?: any;
MathJax?: any;
MozBlobBuilder?: any;
WebKitBlobBuilder?: any;
__fixtures__?: any;
decodeURIComponent?: any;
encodeURIComponent?: any;
Expand Down
97 changes: 97 additions & 0 deletions typings/guppy-defs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/* eslint-disable camelcase */
// Code - third_party/static/guppy-b5055b/src/guppy.js
Copy link
Member

Choose a reason for hiding this comment

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

Wanted to check whether Guppy can also be an auto-upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know what is the proccess of upgrading the libs in manifest.json. Probably @vojtechjelinek can help here.

Also Guppy's repo has been inactive for about 2 years. And we are using the latest version from that repo I guess.

Copy link
Member

Choose a reason for hiding this comment

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

No, it can't since even a minor version needs to be upgraded manually.


interface GuppyConfig {
events?: Object;
settings?: Object;
}

interface GuppyInitConfig {
symbols?: string[];
path?: string;
osk?: Object;
events?: {
ready?: Function;
change?: Function;
left_end?: Function;
done?: Function;
completion?: Function;
debug?: Function;
error?: Function;
focus?: Function;
}
settings?: {
xml_content?: string;
autoreplace?: string;
blank_caret?: string;
empty_content?: string;
blacklist?: string[];
buttons?: string[];
cliptype?: string;
}
callback?: Function;
}

interface GuppySymbols {
symbols?: Object;
templates?: Object;
validate: Function;
symbol_to_node: Function;
split_output: Function;
make_template_symbol: Function;
lookup_type: Function;
eval_template: Function;
add_symbols: Function;
add_blanks: Function;
}

class Guppy {
activate: () => void;
asciimath: () => void;
deactivate: () => void;
doc: () => Object;
equations: () => Array<Object>;
evaluate: (evaluators?: Object) => Object;
func: (evaluators?: Object) => Function;
import_latex: (text: string) => void;
import_syntax_tree: (tree: Object) => void;
import_text: (text: string) => void;
import_xml: (text: string) => void;
is_changed: () => boolean;
latex: () => string;
recompute_locations_paths: () => void;
render: (updated?: boolean) => void;
render_node: (t: string) => string;
select_to: (x: number, y: number, mouse: Object) => void;
symbols_used: (groups?: Array<String>) => string[];
syntax_tree: () => Object;
text: () => string;
vars: () => string[];
xml: () => string;

constructor(id: string, config: GuppyConfig);
}

namespace Guppy {
export function init(config: GuppyInitConfig): void;
export let instances: Object;
export let active_guppy: Object;
export let Symbols: GuppySymbols;
export let Doc: Function;
export function add_global_symbol(
name: string, symbol: Object, template?: string): void;
export function get_loc(
x: number, y: number, current_node?: Object, current_caret?: Object): {
current: Object,
caret: number,
pos: string
};
export let kb: Object;
export function make_button(url: string, cb: Function): HTMLImageElement;
export function mouse_down(e: Object): void;
export function mouse_move(e: Object): void;
export function mouse_up(): void;
export let ready: boolean;
export function register_keyboard_handlers(): void;
export function remove_global_symbol(name: string): void;
}
4 changes: 0 additions & 4 deletions typings/third-party-defs.d.ts
View file
Open in desktop

This file was deleted.