Feature: result grouping by main dictionary sequence (along with some other changes) #95

siikamiika · 2017-10-19T17:50:56Z

I can't find anything major to change or implement, so I'm opening this pull request. Some minor things below:

I'm not sure if compactTags and compactGlossaries should be on by default for new users. They will be off for existing users.
I tested on a Windows Firefox that utilStringHashCode gives the same hash for the first revision of the field templates (it could have messed up with \r\n after formRead). Can we trust this or should the string be normalized before hashing?

The compiled template is already there, though

- use correct tags - indicate popular and rare terms - indicate definitions restricted to specific terms - frequencies (Innocent Corpus)

Alt+P now works again in grouped/split mode In merged mode, 「、」 is added even after the last term, but it's hidden for that. This ensures consistent behavior with voice button and tags

Use ['gloss', 'ary'].concat('DictName') Known collision: 日本国有鉄道 in JMdict and JMnedict

The dictionary tags field can now have a '\t' in it, and it is used to separate tags associated with definitions and terms.

FooSoft · 2017-10-19T20:31:21Z

Thanks for the PR! This is pretty large, so I will be looking this over the next couple of days (probably only get to running my normal tests on it this weekend).

FooSoft

Some initial thoughts on the code, going to actually run it next ; )

FooSoft · 2017-10-21T14:48:45Z

tmpl/terms.html

+                {{/each}}
+            </div>
+            {{/if}}<!--
+     --></div><!--


What's the reason for consuming spaces here? It shouldn't impact the layout and arguably makes the template harder to understand.

These are probably result of me trying to eliminate some whitespace between the term and the preceding comma. Seems like I wasn't aware of ~ at that moment.

FooSoft · 2017-10-21T14:51:43Z

tmpl/terms.html

+                <span class="label label-default tag-frequency">{{dictionary}}:{{frequency}}</span>
+                {{/each}}
+            </div>
+            {{/if}}<!--


An easy way to consume space without making the template more complicated in areas next to handlebars blocks is to use the ~ character. I can't remember for sure if it works with closing tags, but I think you can do something like {{/if~}} to consume all spaces after the end of the if block.

FooSoft · 2017-10-21T14:55:10Z

tmpl/terms.html

 {{#if glossary.[1]}}
-<ul>
+<ul {{#if compactGlossaries}}class="compact"{{/if}}>


Prefer a more descriptive name for this class, so maybe compact-glossary?

FooSoft · 2017-10-21T14:56:34Z

tmpl/terms.html

    <span class="label label-default tag-{{category}}" title="{{notes}}">{{name}}</span>
    {{/each}}
 </div>
 {{/if}}
+{{#if only}}
+<div {{#if compactGlossaries}}style="display: inline-block;"{{/if}}>


Prefer not to use classes instead of style attributes, especially since this one is reused. Maybe something like compact-info or something else to that effect.

FooSoft · 2017-10-21T15:01:21Z

ext/bg/js/util.js

+function utilStringHashCode(string) {
+    let hashCode = 0;
+
+    if (string.length === 0) {


This conditional is unnecessary since if string.length == 0 the for loop will not execute and you will end up return 0 anyway.

FooSoft · 2017-10-21T15:14:19Z

ext/bg/js/options.js

+            options.general.compactTags = false;
+            options.general.compactGlossaries = false;
+            if (utilStringHashCode(options.anki.fieldTemplates) !== -805327496) { // a3c8508031a1073629803d0616a2ee416cd3cccc
+                options.anki.fieldTemplates = '{{#if merge}}\n' +


Prefer template string as it does not require explicit line breaks and you can just use all of the expressions inline without having to combine strings with + operator.

FooSoft · 2017-10-21T15:17:08Z

ext/bg/js/options.js

+            } else {
+                options.general.resultOutputMode = 'split';
+            }
+            options.general.compactTags = false;


You don't need to set compactTags and compcatGlossaries defaults here since they are already specified in the default options object. The first thing that happens when options are loaded is that the default options are merged onto the user options, meaning any missing fields are filled in. The only time you have to manually do things in version blocks is if you want to base settings on a converted version settings used in a prior options version.

Yeah, seems like they are useless at the moment. I wasn't sure if they should be set true by default for new users but false for existing users that haven't had the features in the first place.

Yeah the idea is that you only have to write versions that transform prior data or you are fixing up broken options data that you introduced with a bug, which I have totally never had to do before.

FooSoft · 2017-10-21T15:20:06Z

ext/bg/js/handlebars.js

+function handlebarsTermFrequencyColor(options) {
+    const termFrequency = options.fn(this);
+
+    if (termFrequency === 'popular') {


Probably better to do this with classes. Having to update js files to change color scheme is odd.

FooSoft · 2017-10-21T15:21:37Z

ext/bg/js/dictionary.js

+function dictTermTagScore(tags) {
+    let score = 0;
+
+    const tagScores = {


Maybe I'm missing something, but aren't these actually stored in the metadata in the dictionary JSON?

The score for terms in term_bank_n.json is currently the result of termTags like P or news and definitionTags like arch. Maybe yomichan-import should be updated to have a score separately for both of these. But that would require changes to the sorting. tag_bank_n.json only has [Name, Category, Order, Notes].

In that case, I think it would be fine to introduce Score to the tag array (at the end, as usual). As you've probably noticed, a lot of effort is made to make sure that all data comes from data files only. The only exception is for styling (that's why it is OK that tag colors are in the style sheets).

FooSoft · 2017-10-21T15:24:12Z

ext/bg/js/dictionary.js

+        }
+
+        for (const tag of definition.definitionTags) {
+            if (typeof tag === 'string') {


Why again is it that definitionTags has tags that are either strings or objects? Is there a reason why they aren't objects always?

dictTermsMergeByGloss is used for definitions from Database.findTermsBySequence first (tags aren't expanded) and a mix of definitions from Translator.findTerms (tags expanded) and Database.findTermsExact (not expanded) after that. The tag metadata isn't needed in dictTermsMergeByGloss currently, so I didn't think it was necessary to expand the tags in the rest of the definitions at that point yet.

I think tags should be expanded from the start for everything for clarity sake. It's much harder to reason about code when the type is not explicit. Of course, in languages like JavaScript, explicit type declaration is not an option (unless you use Typescript or something), so we have to rely on convention. By making tags be the same thing everywhere it's a lot easier to write bug-free code. Using typeof should be reserved only for checking if something that could have a false-like value is undefined.

FooSoft · 2017-10-21T17:02:45Z

ext/bg/js/settings.js

+
+    $('.dict-group').each((index, element) => {
+        const dictionary = $(element);
+        if (dictionary.data('title') !== mainDictionaryTitle) {


I don't think that this should be necessary if all of the radio buttons are in the same group (set by name attribute). Setting one should unset the others.

FooSoft · 2017-10-21T17:06:06Z

I'm not sure I like the UX of the radio buttons for setting the "main" dictionary, but I don't yet know what would be better. Radio buttons are usually shown grouped closely together, and the fact that they are so far apart looks odd. Also I notice that after clearing dictionaries (or perhaps even importing dictionaries for the first time), none of the dictionaries are set as "main". If we want to support not having a "main" dictionary, then radio button is not the correct control to use. It should be a dropdown list with the names of all the dictionaries with the first option being set to "None" or something similar.

siikamiika · 2017-10-23T14:16:56Z

I remember that I planned setting the first imported dictionary that has sequence to the main dictionary but failed to find a good way to do it without changing format in index.json. Even in that case there could be 0 dictionaries that have sequence so radio buttons would still be the wrong approach. A dropdown list (that appears when merged mode is enabled?) sounds like a good solution. Also, it would be nice if the dropdown only contained dictionaries that actually have sequence but I can't think of a good way to do this with the current dictionary format.

FooSoft · 2017-10-23T15:34:39Z

Also, it would be nice if the dropdown only contained dictionaries that actually have sequence but I can't think of a good way to do this with the current dictionary format.

This can be achieved by expanding on the data contained in the database:

dictionaries: '++,title,version'

We already store meta information about the dictionaries; all you would have to do is add a column like "hasSequences" and set it to true if all of the sequence data coming out of the dictionary is valid (!= -1). Versioning the dictionaries table is not going to be required since hasSequences does not have to be a database key. You should be able to just grab all the dictionaries and iterate over them and check for the truthiness of hasSequences. Dictionaries imported with older versions of Yomichan will have a value of undefined for the column, which will will work just fine instead of false.

siikamiika · 2017-10-24T21:01:55Z

The problem areas should be better now. The  issue in terms.html actually required some more of them to get fixed completely (but I think I also replaced some with ~). Not sure if Handlebars would have some cleaner way to do it but this is how I've done it in the past because it lets you keep the markup indented.

FooSoft · 2017-10-26T01:37:39Z

ext/bg/js/settings.js

+    titles = titles.filter(title => options.dictionaries[title].enabled);
+    const formOptionsHtml = [];
+    let mainDictionarySelected = false;
+    for (title of titles) {


You probably mean for (const title of titles); not having const here makes it this look kind of wonky.

FooSoft · 2017-10-26T01:39:21Z

ext/bg/settings.html

+                    </select>
+                </div>
+
+                <div class="form-group options-merge">


Please move this under the "Dictionaries" section.

FooSoft · 2017-10-26T01:42:46Z

Looking good 👍

One last thing: I think it would be more visually appealing to have the (Not selected) text appear in a muted color (add the text-muted class to the option, drop the parens).

siikamiika · 2017-10-26T08:13:54Z

Nice! I hope it doesn't break things when released and that users find it useful. It must be confusing that the mode doesn't work with old dictionaries but it should help that they don't show up in the dropdown. Feel free to rename things to make them easier to understand and remove the warning (experimental) if you think this feature is ready.

FooSoft · 2017-10-26T16:59:00Z

🎉 🎈

siikamiika · 2017-10-26T19:19:48Z

👍

siikamiika added 30 commits September 29, 2017 05:41

basic structure for feature-merge-similar-results

b24c705

merged: first draft, only supports JMdict

803ccc1

merged mode: missing changes from previous commit

3b664dd

The compiled template is already there, though

merged mode: implement missing stuff, refactoring

69ad4a7

- use correct tags - indicate popular and rare terms - indicate definitions restricted to specific terms - frequencies (Innocent Corpus)

merged mode: remove extra whitespace between terms

03fd9dc

merged mode: add main dictionary selection

cfad3b3

merged mode: merge other results with main dict

72fe83d

merged mode: rewrite previous commit

981d9ed

merged mode: support audio and tags for terms

94d590f

merged mode: fix regression (voice), tweak hover

7b9ad61

Alt+P now works again in grouped/split mode In merged mode, 「、」 is added even after the last term, but it's hidden for that. This ensures consistent behavior with voice button and tags

merged mode: fix tags for non-main dictionaries

84420e0

merged mode: support frequencies

9f277b6

merged mode: support audio hotkey Alt+P

b9d5893

merged mode: make gloss hash more unique

e1c5d7a

Use ['gloss', 'ary'].concat('DictName') Known collision: 日本国有鉄道 in JMdict and JMnedict

add termTags

4fb983a

The dictionary tags field can now have a '\t' in it, and it is used to separate tags associated with definitions and terms.

reset main dictionary on purge

7783aa4

merged mode: remove options.dictionary.main

4e57fa1

pos tag color

6523b77

add compact tags

8d660e2

compact tags: reset PoS on new dictionary

c0f5648

change db format, rename tags to definitionTags

7ec28be

rename pos to partOfSpeech

4203fda

merged mode: add secondary searches

03f7ca2

compact glossaries, line break after tags: on/off

0404b42

line break after tags on by default

293606d

scrap tagLineBreak option

7913b09

merged mode: fix OBOE (sequence 0)

b104dfd

merged mode: tags -> termTags (inside expressions)

1edeabd

merged mode: Anki support

81f0a61

merged mode: support {audio} marker (first term)

c1c851d

siikamiika added 2 commits October 19, 2017 19:27

Anki: term frequency colors in merged mode

bc92327

...with {furigana-plain}, too

bf15358

FooSoft reviewed Oct 21, 2017

View reviewed changes

siikamiika added 10 commits October 24, 2017 12:38

add tag meta score

e034ca3

Anki: use CSS classes for term frequency color

8400f0e

database.js: add hasSequences

df0d2be

settings: main dictionary selection as dropdown

78442ff

expand tags before dictTermsMergeByGloss

27a56dc

fixups: remove redundant options, use template str

c9b1dd5

translator.js: coding style

73d5425

util.js: remove unnecessary conditional

6f43fff

terms.html: rename classes, remove inline css

9697c9a

terms.html: fix whitespace issues in merged mode

f828043

merged mode: restore whitespace between termTags

ee80fa5

FooSoft reviewed Oct 26, 2017

View reviewed changes

siikamiika added 3 commits October 26, 2017 10:18

merged mode: restore whitespace between frequencies

de28190

settings: add missing const

e063a31

settings: move main dictionary below Dictionaries, styling

ab1e2b3

FooSoft merged commit 25ae2e4 into FooSoft:dev Oct 26, 2017

siikamiika mentioned this pull request Jan 28, 2020

Bug with termTags and Translator data format #337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: result grouping by main dictionary sequence (along with some other changes) #95

Feature: result grouping by main dictionary sequence (along with some other changes) #95

siikamiika commented Oct 19, 2017

FooSoft commented Oct 19, 2017

FooSoft left a comment

FooSoft Oct 21, 2017

siikamiika Oct 23, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

siikamiika Oct 23, 2017

FooSoft Oct 23, 2017

FooSoft Oct 21, 2017

FooSoft Oct 21, 2017

siikamiika Oct 23, 2017

FooSoft Oct 23, 2017

FooSoft Oct 21, 2017

siikamiika Oct 23, 2017

FooSoft Oct 23, 2017

FooSoft Oct 21, 2017

FooSoft commented Oct 21, 2017

siikamiika commented Oct 23, 2017

FooSoft commented Oct 23, 2017

siikamiika commented Oct 24, 2017

FooSoft Oct 26, 2017

FooSoft Oct 26, 2017

FooSoft commented Oct 26, 2017

siikamiika commented Oct 26, 2017

FooSoft commented Oct 26, 2017

siikamiika commented Oct 26, 2017

Feature: result grouping by main dictionary sequence (along with some other changes) #95

Feature: result grouping by main dictionary sequence (along with some other changes) #95

Conversation

siikamiika commented Oct 19, 2017

FooSoft commented Oct 19, 2017

FooSoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FooSoft commented Oct 21, 2017

siikamiika commented Oct 23, 2017

FooSoft commented Oct 23, 2017

siikamiika commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FooSoft commented Oct 26, 2017

siikamiika commented Oct 26, 2017

FooSoft commented Oct 26, 2017

siikamiika commented Oct 26, 2017