Add "make anki note" buttons to displayed articles. #449
Conversation
article_maker.cc
Outdated
| @@ -642,19 +665,56 @@ void ArticleRequest::bodyFinished() | |||
|
|
|||
| closePrevSpan = true; | |||
|
|
|||
| head += string( R"(<div class="gddictname" onclick="gdExpandArticle(')" ) + dictId + "\');" | |||
There was a problem hiding this comment.
I had to partially refactor this because this was almost unreadable and unmaintainable. I used QString because it has the arg() method that can be used to easily mimic libfmt.
|
This will never work reliably. A lot of HTML dictionaries (most mdx & zim ones) relies on their CSS & JS to display correctly. If someone complains, we cannot fix it. |
Are there any examples of these that we can test? Currently the existing context-menu action gets the definition from the selected text, and doing so always strips both CSS and JS. Yet it seems to work fine. I think it's an easier path to send definitions as plain text to Anki since Anki cards are supposed to be simple. Sending HTML will lead to a number of problems, such as broken elements and a lot of bloated, unreadable markup.
However, if we find a way to clean HTML before sending it to Anki, it will bring more possibilities for styling/formatting. |
article_maker.cc
Outdated
| gd_dict_name += R"EOF( | ||
| <a href="ankicard:%1" class="ankibutton" title="%2" > | ||
| <img src="qrc:///icons/add-anki-icon.svg"> | ||
| </a> | ||
| )EOF"; | ||
| gd_dict_name = gd_dict_name.arg( | ||
| dict_id_html.c_str(), | ||
| tr( "Make a new Anki note" ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
this can use a style to hold the place and use css to control the display.
pseudocode:
html:
<div class=anki-button> </div>
css:
.anki-button{
background-img:xxx
}
The following is my guess(use if it is easier and suitable)
there is no neccessary to pass dict_id to the ankicard:%s.
check the function about how to select current dictionary . When click the button ,get the dictionary id which hold this button .which will need a javascript event listener for .ankibutton
There was a problem hiding this comment.
this can use a style to hold the place and use css to control the display.
The above code uses <img> inside a <span> which seems reasonable.
<span class="collapse_expand_area" onclick="gdExpandArticle('%1');" >
<img src="qrc:///icons/blank.png" class="%2" id="expandicon-%3" title="%4">
</span>
For the anki add button we decided to follow the same pattern. Of course, alternatively it's possible to have "qrc:///icons/add-anki-icon.svg as a background image, but then we'd have to explicitly style the <a> tag, adding quite a lot of additional CSS (size, background position, repeat, etc.). I have no preference but it seems the current solution is easier.
However, I think it's a good idea to use css to control the visibilty (display:none if ankiconnect is disabled). I'll try to implement that.
there is no neccessary to pass dict_id to the ankicard:%s.
I think you're right, but dict_id_html is already used quite a number of times throughout the code so it's easier to pass it right away instead of fetching the article's id later with additional JavaScript.
There was a problem hiding this comment.
However, I think it's a good idea to use css to control the visibilty (
display:noneif ankiconnect is disabled). I'll try to implement that.
I think if not enabled,maybe do not output the placeholder
article_maker.cc
Outdated
| contexts, | ||
| ftsDicts, | ||
| header, | ||
| -1, |
There was a problem hiding this comment.
I have hold a global cfg in Globalbroadcaster
article_maker.cc
Outdated
| bool const ankiConnectEnabled = GlobalBroadcaster::instance()->getPreference()->ankiConnectServer.enabled; | ||
| gd_dict_name += R"EOF( | ||
| <a href="ankicard:%1" class="ankibutton" title="%2" style="%3" > | ||
| <img src="qrc:///icons/add-anki-icon.svg"> | ||
| </a> | ||
| )EOF"; | ||
| gd_dict_name = gd_dict_name.arg( dict_id_html.c_str(), | ||
| tr( "Make a new Anki note" ), | ||
| ankiConnectEnabled ? "" : "display: none;" ); | ||
|
|
There was a problem hiding this comment.
if not enabled ,I think there is no need to output this section.
There was a problem hiding this comment.
Alright. I'll change it to a lambda since that seems like the only way to avoid the code smell warning.
article_maker.cc
Outdated
| gd_dict_name += R"EOF( | ||
| <span class="collapse_expand_area" onclick="gdExpandArticle('%1');" > | ||
| <img src="qrc:///icons/blank.png" class="%2" id="expandicon-%3" title="%4"> | ||
| </span> | ||
| )EOF"; | ||
| gd_dict_name = gd_dict_name.arg( | ||
| dict_id_html.c_str(), | ||
| collapse ? "gdexpandicon" : "gdcollapseicon", | ||
| dict_id_html.c_str(), | ||
| collapse ? tr( "Expand article" ) : tr( "Collapse article" ) | ||
| ); |
There was a problem hiding this comment.
the gd_dict_name is becoming larger .
use a temp string to hold the string should make the code more simple.
pseudocode:
collapseHtml=R"xxxxxx".arg();
gd_dict_name+= collapseHtml
There was a problem hiding this comment.
I thought that immediately applying arg() would severely hinder readability.
There was a problem hiding this comment.
current gd_dict_name both act as arg() operation and concat ,and a concat must follow the arg() operation.
the operations depends on each other and must follow order.
it would easily cause trouble when others changes code .
for example ,add some extra html to the current html structure.
or add extra parameter to arg() but forget to provide the value.
There was a problem hiding this comment.
I have changed each clause to an immediately invoked lambda. Visually and semantically this should signal that each clause is a single unit.
tatsumoto-ren
force-pushed
the
new_card_button
branch
from
b087a15
to
bcc0920
Compare
|
Personally, I feel putting the "make anki note" (or "Send article to Anki") on the right click menu makes more sense: 1.) We already have those on right click menu
"Send article $word to Anki" looks like align with them pretty well. With previous experience, a user may naturally deduce that extra Anki related functionality also on right click menu? 2.) If the height of an article is larger than 1 screen, user must scroll back then click the icon. I think most people will naturally move cursor to a place near where they are looking at while reading. Accessing right click menu is faster and easier to click? 3.) A few ArticleDisplayStyles, Modern & Lingvo & Lingoes, has the "Collapse" button on the right side. Not sure how this new icon will handle. "Collapse" and another button on the same side doesn't feel right. The titlebar of Modern & Lingvo & Lingoes also doesn't span the entire line. They are supposed to be as short as possible? Adding an icon on them doesn't feel right. Relevant art: Firefox's right click menu + extra functionalities.
|
Honestly I think that's a pretty bad idea from the UI perspective because it forces the user to hunt for menus. Also, GoldenDict already has an enormous amount of right-click actions, and personally I find it a bit difficult to find the right one. A button is clearly visible, can be easily found and requires only one click instead of two to use. The key idea of this PR is to introduce a feature that already exists in Yomichan, and has been there for years, so it's a working and tested solution. Yomichan users are familiar with it and use it every day.
But I think adding a keyboard shortcut would be a good idea too. Even though it's not visible right away, at least it doesn't require multiple clicks. Ctrl+N seems like a reasonable candidate because that's what Mpvacious has.
As an additional feature this might be okay. |
|
Looks like Ctrl+N was already taken so I assigned Ctrl+Shift+N (N stands for new) as a new keyboard shortcut for making anki cards. |
but we are not Yomichan and not a duplication of it. Yomichan is a popup inside a webpage, so it cannot have a right click menu. The interface is limited to the popup and nothing else. In contrast, GD has a mainwindow with tabs and sidebars. The default dictionaries provided by Yomichan's website are short, while a lot of dictionaries on goldendict are much longer. On other article display styles, the 🟢 is ugly and weird. If someone enables anki and uses those styles, an issue will for sure be filed about how they want the 🟢 gone, and we have no way to fix it.
The right-click menu only has 3 entries when right-click an empty space, it is definitely not "enormous"
but crowding the main interface is also a distraction. Advanced features are less used. If we put everything on the toolbar, it will be too much for most people.
Yes, but the one click requires travelling the cursor to a corner (plus some scrolling if the dict is long) instead of consecutive 2 clicks in place. I think right click menu is good enough to use, and the dict title bar should not be bloated with extra icons. |
This is not a reason to avoid introducing a feature that works well in other dictionaries, yomichan in this case.
I don't use the popup either, yet I'd like to have a button for making cards.
A lot of dictionaries for Yomichan are quite long because they are converted from EPWINGs. They are also grouped together so each entry has definitions from a number of different dictionaries. On top of that the popups are quite small, so I imagine it's not uncommon for the
Looks pretty good to me. I don't see why this would be a problem.
And if there is selected text, the right-click menu is filled with a lot more items. This is a little inconvenient. screencast-area-2023-Apr-04_16-52-19.mp4
You still have to move the cursor to the right context-menu item. So there's 3 actions instead of 2 (click-move-click vs move-click). If having a button (which can be disabled by the way) is such a bad idea, I think I would be okay with a right-click action as long as there's a shortcut assigned to it. |
It is weird. On this style, click the red region will collapse the article, and in the middle of it you can save the aritcle to somewhere else.
If you rearrange the icons, it is still weird. The arrow indicator pointing to a plus sign. The "save to anki" is not part of "title region that can be clicked to collapse the article".
Only in styles that span the entire line, the 🟢 looks OK.
Also, this style is broken.
|
|
I wanna know what @xiaoyifang thinks. If a keyboard shortcut is better than a button, I'm going to make another PR that introduces just the shortcut and close this one. |
|
I am minimalist. Probably, I am biased against extra buttons. Adding an extra right click menu and a shortcut should be good enough for everyone. Thank you for your understanding. |
@shenlebantongying always have good sense about the functionality about the goldendict. what's about this workaround , we can also have the right context menu and shortcut in the same time. we can give it a try and accept more user feedback. If conditions acquires ,we can add a new option below the Anki 's host |
I will move it under the heading and make it float to the right. |
the anki button is shown under the heading
to reduce size of the diff
|
SonarCloud Quality Gate failed.
|
This PR adds a "make a new anki note" button to each article if the user has configured Anki integration in Preferences. Pressing the button results in a new Anki note being added to one's Anki deck, with the corresponding article's text as the definition. If there is selected text, it will be sent instead, similarly to the context menu action that was added before.
Having a button allows to mimic more closely the process of creating Anki cards with Yomichan. Yomichan also places a
+button next to each search result on the right side.Screencast:
screencast-2023-Apr-03_18-55-43.mp4