davidsansome / tsurukame

Tsurukame is an unofficial WaniKani app for iOS. It helps you learn Japanese Kanji.
https://tsurukame.app
Apache License 2.0
249 stars 58 forks source link

Examples not always showing translation #695

Open jaherrera5 opened 5 months ago

jaherrera5 commented 5 months ago

IMG_8723

In the image it I screenshot (iPhone currently on iOS 17.2.1 on an iPhone max pro 14) the context sentence goes over the phone screen width and so it will blur the second half of the Japanese sentence and then not show the translation. This is the first time I see it, or least the first time I’ve noticed it. The “す。” portion of the second example was initially blurred out and when I clicked it to unblur it, it just showed the “す。” as is now in the image, but no translation still.

Edit: it happened again. I’m adding the new image for reference. IMG_8724

IMG_8725

New edit: looks to be specific to when the Japanese sentence only barely flows into the new sentence once unblurred. When blurred, it’s on one line. Once unblurred the very end of it goes to the second line. This new image doesn’t have that issue and it is a two-liner as well.

jaherrera5 commented 5 months ago

Looking at the code, I believe the issue is in/and around ContextSentenceModelItem.swift line 37 where you append the '/n' newline after generating the Japanese sentence with the highlightOccurrences function. My best guess is that the space allocation for the sentence is determined prior to adding the '/n' character for the highlighted Japanese sentence and if it is exactly the size of the width, it will then take up the next line with the unblurring. Therefore, the '/n' appended afterwards will make it go onto the line designated for the English translation, hence, covering it up. I only shortly scanned the code so I can't say this for certain, plus I don't know swift myself, but this looks to be the reason why or at least in this area.

My idea as of right now: If the allocation for the Japanese is done after the '/n' is added to the Japanese text (maybe as simple as adding the '/n' within the "VocabularyHighlighter.swift" file to take into account the '/n' during the highlighting process), then it might resolve the issue. If there is a specific reason for not doing it within the VocabularyHighlighter.swift file (such as somewhere else it needs to not have the '/n') then it maybe as simple as creating a tmpJapanese var to create the Japanese with the appended '/n' and then call the highlightOccurrences function once the japanese variable has the '/n' character appended. However, either of these may potentially make it have a blank line between the Japanese and blurred English visually on the app, but I don't think this will happen. As mentioned, I did a quick view of the code so I can't say this for certain, but I can do a PR later in the week and see about adding that to the code.

jaherrera5 commented 4 months ago

@UInt2048 Doesn't look like I have permissions to make a push. But the change was slightly in a different place. In file ContextSentenceModelItem.swift on line 54, you use the "japanese" variable to initialize the "japaneseText" variable. The "japanese" variable does not have the appended '\n' yet. It would be as simple as adding the following just prior to the "japaneseText" initialization:

japanese.append(attr("\n")) // inserted at line 54 japaneseText = NSMutableAttributedString(attributedString: japanese).replaceFontSize(fontSize) // now line 55

UInt2048 commented 4 months ago

@jaherrera5 The way it works on GitHub is you fork the repo, commit your changes to some branch on your fork (which you do have permission to push to), and then make a pull request on this repo.

UInt2048 commented 4 months ago

@jaherrera5 Also, if this is in fact a tested fix (not sure if Xcode would complain about this — japanese is a constant variable on line 37), wouldn't it be easier to just replace:

https://github.com/davidsansome/tsurukame/blob/f77ed0807150a99450733e2e34b8ca4676b423e9/ios/Tables/ContextSentenceModelItem.swift#L37-L40

With something like:

var japanese = highlightOccurrences(of: highlightSubject, in: attr(sentence.japanese)) ?? 
  attr(sentence.japanese)
japanese.append(attr("\n"))
text.append(japanese)

Curious though why this issue would only show up now though if this code has been used seemingly without problems for several years... 🤷🏻

jaherrera5 commented 4 months ago

@UInt2048 Oh, I haven't tested it. Apologies. I made a separate branch and was going to push and hopefully you could test it, but I really should test it myself beforehand as you mentioned. I don't use github at work so I'm getting a feel for it. As I mentioned, I've never worked with swift, nor have I ever even owned an apple computer. A while back I tried working with Objective-C a bit, but I've never tried Apple dev since Swift came along. Might be a nice project for this weekend though. I'll give it a try. As for why, not sure. Weirdest part, yet at the same time sort of makes sense looking at the code, is how the Japanese text is all on the top line initially - no English translation showing below it (not even blurred) - then when you tap on it to "unblur" what is not there the last character and the period jump down to the next line. Still happening today. The same Kanji characters in the provided images I went to in the app and they had the same issue still. So it's repeatable and not a one off situation.