Skip to content

Commit

Permalink
#7 Solve warnings "duplicate declaration"
Browse files Browse the repository at this point in the history
  • Loading branch information
@kimkha
kimkha committed Nov 4, 2014
1 parent c44fd23 commit fd82117
Showing 1 changed file with 7 additions and 5 deletions.
12 changes: 7 additions & 5 deletions src/scripts/avim.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@
var replaceBy = fcc(c), wfix = upperCase(unV(fcc(c)));
AVIMObj.changed = true;
} else {
var replaceBy = c;
//var replaceBy = c;
if((upperCase(c) == "O") && AVIMObj.whit) {
bb=true;
}
Expand All @@ -582,9 +582,11 @@
if ((upperCase(o.value.substr(pos - 1, 1)) == 'U') && (pos < savePos - 1) && (upperCase(o.value.substr(pos - 2, 1)) != 'Q')) {
if((wfix == "Ơ") || bb) {
if (o.value.substr(pos-1,1) == 'u') {
var r = fcc(432);
//var r = fcc(432);
fcc(432);

This comment has been minimized.

Sign in to view

Copy link
@1ec5

1ec5 Nov 8, 2014

Collaborator

Are you sure? This probably breaks things like guongw → gương, because r is used on line 602. In non-strict JavaScript, that’s perfectly valid, so the declaration of r here actually does have an effect.

This same code also appears on lines 609 and 611, by the way.
} else {
var r = fcc(431);
//var r = fcc(431);
fcc(431);
}
}
if(bb) {
Expand Down Expand Up @@ -1135,9 +1137,9 @@
function _keyDownHandler(e) {
if(e == "iframe") {
AVIMObj.frame = findF();
var key = AVIMObj.frame.event.keyCode;
//var key = AVIMObj.frame.event.keyCode;
} else {
var key = e.which;
//var key = e.which;
}
}

Expand Down

9 comments on commit fd82117

@1ec5
Copy link
Collaborator

@1ec5 1ec5 commented on fd82117 Nov 8, 2014

Choose a reason for hiding this comment

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

Hiếu’s code makes use of var hoisting quite a bit. The let keyword can’t be hoisted, but Chrome unfortunately lacks support for it (unless you enable the “Experimental JavaScript features” flag).

If you’re going to silence strict warnings, I suggest transitioning to strict mode. Strict mode prohibits some of the antipatterns of JavaScript, but it also changes the script’s semantics in subtle ways, so be careful and test thoroughly. When I migrated AVIM for Firefox to strict mode and the let keyword, I introduced regressions in findC() and elsewhere.

@kimkha
Copy link
Owner Author

@kimkha kimkha commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

Sorry, I found the issue...

Not this one: guongw will become gương.
But: guowng will become guơng

@1ec5
Copy link
Collaborator

@1ec5 1ec5 commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

That’s always been the case with AVIM; it isn’t a result of this change. If you put a tone mark on guơng, the u will turn into ư, for example guơngJ → gượng.

@kimkha
Copy link
Owner Author

@kimkha kimkha commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

Oh, so I guest with newest version, the issue gone. But it still have problem because user maybe write guowjng for gượng (I guest so).

Do you have any idea?

@1ec5
Copy link
Collaborator

@1ec5 1ec5 commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

I think that would be a common way to write “gượng”. It works in avim.js and 1ec5/avim but not kimkha/avim-chrome. Not sure if that regression is due to this change. But in any case, please consider uncommenting the lines and moving the var keywords outside the if blocks.

@kimkha
Copy link
Owner Author

@kimkha kimkha commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

In the newest version I already moved var keyword outside the if blocks, but it still not work...

By the way, do you have time to help me improve that extension? I will add you as collaborator.

@1ec5
Copy link
Collaborator

@1ec5 1ec5 commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

Ah, OK. It would probably take some debugging to figure out what’s going on, then. At some point, it might be worth adapting the Firefox extension’s architecture, which isolates the gnarly core logic into a separate script (transformer.js) that’s easy to unit test and debug inside a browser window.

Honestly I don’t think I’ll have much time for either AVIM in the near future. But eventually I’d love to help bring some of the Firefox extension’s improvements over to this project. (Not all of them are possible with Chrome’s extension model, unfortunately.)

@kimkha
Copy link
Owner Author

@kimkha kimkha commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

I also look over avim firefox, and I found it is very nice structure and have a good test architect. And I hope avim-chrome will be the same architect with avim firefox, that will be more easier for new contributor to join both of us.

I already add you as collaborator with pushing permission.

Thank a lot.

@kimkha
Copy link
Owner Author

@kimkha kimkha commented on fd82117 Mar 2, 2015

Choose a reason for hiding this comment

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

I also improve layout of https://github.com/1ec5/avim/wiki/Architecture. Please check it.

Please sign in to comment.