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.

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

BUG: Search Field Too Wide & Hides Icons: Need Style Purge Feature #213

Closed
washere opened this issue Nov 27, 2021 · 33 comments · Fixed by #215
Closed

BUG: Search Field Too Wide & Hides Icons: Need Style Purge Feature #213

washere opened this issue Nov 27, 2021 · 33 comments · Fixed by #215
Assignees
Labels
bug fixed Fixed, awaiting publication in new apps
Milestone

Comments

@washere
Copy link

washere commented Nov 27, 2021

Installed the MS Store version.
Downloaded one of the R-to-L ZIM files from app's repo.

Went back to app's small default wiki, the search field is so wide, the top_right icons can not be seen.

Long time coder & UI designer.

Tried:
Many re-installs
Many pref changes in settings, plus many combo of
Downloaded several more ZIM files from app repo
etc.

NO GOOD.
Even after reinstall the app seems fine. But when you close & relaunch app, same problem.
Uninstalled (Windows settings REMOVE APP) many times & reinstalled from MS Store, no good, same bug.
Tried the other version from website but totally different and no dark modes nor dark_article mode. Most coders need dark mode.

Files in the old version (install-able) seem to be in appdata roaming from googling. But MS Store version writes to WindowsApps in prog files folder.
Uninstall removes all folders (usually 4).
Had installed WikiMed & WikiJourney from MS Store too, same bug happens to them. Uninstalled them too, no good for Kiwix re-installs.

Had a look at RegEdit, lots of references. This is a bad practice. I suggest all app makers use the old ini files in their install directories, Registry is a bad idea.

Bug seems to be the odd wiki download set some style settings that persist (probably in Registry, maybe somewhere else).

SOLUTION:
Add New Feature in Preferences/Settings:

PURGE ALL PREVIOUS STYLES & RESTORE DEFAULT STYLE

This will save the developers from not just this bug, but many future bugs, i promise that. So save yourself many future headaches.

QUICK FIX:
Maybe just setup a var to see if style has changed and just reset that.

default_style = 1

App checks that on launch and if value is 1 then it will not load other styles.
If it is 0 then it will load whatever style the user had set or a ZIM file set.

But you need a procedure to completely flush the style changes.

If i change zoom with CTRL and + or -, then the bug goes away. But it is a pain to do this on each launch.
This bug needs fixing anyway.

bug_kiwix

bug_kiwix2

@kelson42 kelson42 transferred this issue from kiwix/kiwix-desktop Nov 28, 2021
@washere
Copy link
Author

washere commented Nov 29, 2021

I think it is some maximum width setting in Registry set by some Wiki files listed in the app's Repository.
These should be reset on a fresh install.
I think potentially a lot of people are suffering from this and many of them do not know CTRL & -/+ temporarily fixes it and just put up with this nasty bug by painfully scrolling to the right constantly.

I tried all options with maximum width in settings before but it does nothing.
Basically all style settings need to be flushed clean on a new install, specially with respect to width settings on every launch which is what this bug is.

I tried installing from command line as admin with winget (winget install kiwix.kiwixjs), no good, bug remains.

@washere
Copy link
Author

washere commented Nov 29, 2021

So i went to portable electron section.
Top sections are for WikiKMED & WikiJOURNEY, scroll down to older version:
Kiwix JS UWP and PWA 1.7.8

https://github.com/kiwix/kiwix-js-windows/releases

clicking on:
NWJS/Electron versions
https://kiwix.github.io/kiwix-js-windows/kiwix-js-electron.html

redirects to:
https://github.com/kiwix/kiwix-js-windows/releases/tag/v1.7.7E

I installed the Win 7/8/10/11 version for portable electron.
This version runs fine on relaunch and the bug goes away (correct width on relaunch) but no self update for the app.
So i have both versions, this portable electron 1.7.7E and the latest version from MS Store with the bug,
I will be using the old portable electron 1.7.7E untill this bug is fixed.
I recommend others do the same, most of the afflicted will not be reading github probably, hope the bug gets fixed and they don't leave feedback on MS Store.

Having 2 versions is good, can load each with a different wiki file for fast access.
Thanks again, waiting for fix.

@washere
Copy link
Author

washere commented Nov 29, 2021

I noticed something today:

I have an external monitor attached to my Dell XPS 13 i7.
If i launch the app on the screen of laptop itself, the bug does not occur on launch.
If I launch the app on the screen of external monitor, the bug occurs.

The app launches itself on the LAST SCREEN IT WAS EXITED FROM.
So if i exit app on laptop screen, it re-launches there, and if i exited on monitor screen, it re-launches on monitor screen (with width resized bug).

The app is pinned to my taskbar, does not matter which taskbar screen (laptop or monitor) i launch it from.

The bug seems to be related to screen resolution.
As i drag a correctly display instance of app from laptop to monitor screen, the bug occurs (crops the top_right icons).

So the fix should be looking at how to resize the top UI libkit elements correctly on a different resolution screen.
Because other apps do this basic function without any problem.

Hope this helps, seems this will affect a lot of people in future.
Thanks again.

@washere
Copy link
Author

washere commented Nov 29, 2021

The bug is quite serious because:

If you resize the app window, drag right side of window to increase the width, the top_right icons stay hidden.
You have to scroll all the way to the right to see icons.

Another sign that this is a UI bug:
Once you scroll to the end of right edge and see icons, even if you reduce the window size to a very small window, the icons are seen & bug goes away.

@Jaifroid
Copy link
Member

Jaifroid commented Nov 29, 2021

@washere I'm the main developer of this version of Kiwix, so I'll be happy to look into this and work with you to resolve it, whether through a style purge option or fixing the underlying issue. I have a specific question: you mention in the top post that you were using the app with R-to-L ZIMs. Is that related to this bug? To be honest, the app hasn't been properly coded for Right-to-Left languages, though in principle the orientation of the ZIM contents should not have an effect on the orientation of the interface (which is L-to-R). The screenshots you give are all with the included ZIM, so perhaps the L-to-R is not central to this bug? I have not seen this particular bug before, but I'll try to reproduce if you can give me some steps. In the meantime I can tell you how to purge all the settings (subsequent post).

@Jaifroid
Copy link
Member

@washere Below is the classic way of resetting a UWP app's data. Go to Settings -> Apps and Features and then follow screenshots. Are you able to do so? I have to say that I'm currently getting a message "We couldn't reset this app, please try again in a bit", at least on Windows 10 (I'll try on Windows 11 separately). If it doesn't work, I may have another way.

image
image

@washere
Copy link
Author

washere commented Nov 29, 2021

@Jaifroid
Thanks for your kind reply.

Before i reply I just noticed something. Correction to my last post:
Scrolling to the right to see icons on the external monitor does not fix the bug (icons still not visible).
The only way to fix this is CTRL & + or -, as i said in 1st post.
I also noticed a 2nd way to fix the bug:
DRAG THE WINDOW FROM EXTERNAL MONITOR TO LAPTOP.
This also fixes it.

Now to answer your question:
I am not using R-to-L zim files. I just tested one on first install to check app's repo files, i downloaded several from repo, only one was R-2-L.
I reinstalled over ten times from MS Store with various changes. I never downloaded a R-2-L file after the first time.
Frankly i think R-2-L is irrelevant.
I think this bug is related to:

  • External monitor display connected to a laptop
  • The external monitor needs to have a larger resolution probably, to replicate the bug (if external is the same resolution or smaller, this bug will not happen in my opinion)

So you need such a setup.

I just saw your 2nd post on how to reset and did that.
The bug remains.
See the 2 pictures, first one is on laptop screen and ok, second one is when i drag it to external monitor and the bug is there.
When i exit the app on laptop screen and relaunch it is ok.
When i exit the app on external monitor screen and relaunch it, the bug recurs again.

So it has nothing to with the only R-2-L file i installed over 10 installs ago.
And it has nothing to do with purging settings.
Seems a more serious problem with external monitors that have higher resolution as i said today above:

#213 (comment)

If you do any test versions and upload it somewhere (mediafire or zippyshare or an ftp URL or etc), i will be happy to test and report back.

Thanks for your attention and great work.

Laptop screen after purge:

laptop_screen

Monitor screen after pruge:

monitor_screen

@Jaifroid
Copy link
Member

@washere Thanks for testing that, and sorry that a reset doesn't fix the issue. It does indeed sound like a screen resolution issue, and possibly something to do with not detecting correctly the resolution of external monitors, so it gets stuck somehow with the resolution of the base monitor.

We use Bootstrap as the UI framework, but it is now a slightly older version of Bootstrap, so I think part of a longer term fix might be to upgrade Bootstrap to the latest version.

However, it sounds like a faster interim fix is possible: to force a screen recalculation (which is what I believe happens when you use the zoom options). I think it's forcing the webview to do a recalculation at quite a low level, because my feeling is that the underlying browser engine is incorrectly reporting the screen dimensions to the framework (Bootstrap), but resets itself when you do a low-level zoom operation.

Could I ask you to test two more things before I try to force that recalculation?:

  1. Does the bug get fixed if you use the + and - buttons that appear at the bottom of the article page (in the app's footer when viewing an article)?

image

  1. Does the bug get fixed if you increase and decrease the slider in configuration "Change the font size for the user interface"?

image

@washere
Copy link
Author

washere commented Nov 29, 2021

Thanks @Jaifroid

The lens + and - icons do not fix it. I tried again now.
I have coded for decades and tested many apps so that was one of the first things i tried yesterday.
I tried many other things too.

The only thing that fixes it is if i use CTRL & + or -.
And also exiting app on monitor screen.
Resolution reset shortcut, CTRL 0, does not fix it. But after CTRL +/-, it does reset correctly to 100%.

Never had this problem with any other app.
I think you are right in the bug being an old bootstrap libkit (good choice) & needing dynamic forced res recalculation. Upgrading would be great and also future-proof.

As i said i tried many other things too yesterday.
If you need me to try anything else or a test build, let me know.

Thanks again.

@washere
Copy link
Author

washere commented Nov 29, 2021

So big or small, on external monitor problem remains:

big

small

@Jaifroid
Copy link
Member

Jaifroid commented Nov 29, 2021

OK, that's useful to know, as it means that the internal recalculations the app does when pressing the buttons are not enough, and it is indeed something lower level. So I need to find a way to force that resize programmatically, for which there should be an API. I'll have a look.

Just to comment on something else you mention in the chain above. We don't explicitly code for using the Registry. In fact all our settings are made in pure JavaScript, and stored in standard Local Storage (with a fall back to Cookies for browsers that don't support Local Storage). This is why we can adapt the app to Electron, NWJS, PWA or UWP frameworks very easily: there is no low-level stuff (with the exception of some framework-specific JS, such as Node for Electron, to allow us to access the ZIM file without the user having to pick it every time). Any Registry entries related to Kiwix JS would have been put there by the framework.

Because the Store app and the Electron app use a different underlying browser engine, this may account for the fact that the Electron app does not exhibit this bug.

Incidentally, you may like to try the PWA version while waiting for a fix. It is available simply by visiting https://pwa.kiwix.org/ . In Chrome or Edge you can install this and it will mostly act just like any other installed app. The only exception is you need to give single-click permission to access the last-loaded archive on relaunch of the app. The PWA version keeps itself up-to-date, and works offline, but could get wiped with a browser reset, so is not so robust for pure offline work as the UWP or Electron apps are.

@washere
Copy link
Author

washere commented Nov 29, 2021

Thanks, I did bookmark PWA url yesterday, it gives me a 3rd instance next to this version & 1.7.7E electron. I am using the latter 2 loaded with 2 different zim files. If i need a 3rd file, i'll use the chrome.

Thanks for the info on the the temp files being local not registry based with cookies fallback for using browser based versions. Good strategy to keep things clean, and also good for read-only systems as in many organizations using the browser version with cookies.

I just now tried many options with intel display graphics settings app, some changes seem to work, but after a few relaunches the bug recurs. So a coding fix seems to be the only persistent solution.

Thanks again and let me know if you want me to test or upgrade.
Great implementation overall, love it, specially as preferring the self upgrade version.
Have a good week.

@washere
Copy link
Author

washere commented Nov 29, 2021

I just noticed something.
As i said the electron version (v1.7.7E) does not have this bug.
But i think it avoids it by launching on the laptop screen. Even if you move it to external monitor screen & resize & exit.

On relaunch, unlike this version, the electron version always launches a new instance on "PRIMARY DISPLAY".
Even if i change the laptop screen from #.1 to #.2 in Windows Display settings, it will launch on laptop screen.

If the app can be forced to launch on primary display screen, i think there will be no need for dynamic recalculations (lot of work & making code bigger & heavier on resources etc).

I think this will be a cheat but the app will remain snappier too and no one will notice any problems.
I don't know if this, launching only on primary (computer's own screen) display is possible with the current code & Bootstrap version.

If i notice anything new, will post.
Thanks again.

@Jaifroid
Copy link
Member

OK, thanks for the pointers. I have now been able to reproduce your issue by using another PC as a wireless display, projecting from my main PC. Result is exactly as you describe for the UWP app, while in the PWA in Chromium (Edge) the app detects the change of screen resolution correctly. I can now debug this, and try out different options. From my experiment, when I launch on my primary display, and then drag it to the secondary screen, the bug occurs (because no change of resolution is detected). Therefore I don't think that forcing it to launch on one screen or other will fix the issue. However, there is a way to detect changes of device resolution which works with this browser engine. See https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio . With this I can add an event listener and hopefully do something when a change of resolution is detected.

image

@Jaifroid Jaifroid self-assigned this Nov 29, 2021
@Jaifroid Jaifroid added the bug label Nov 29, 2021
@Jaifroid Jaifroid added this to the Release 1.8.0 milestone Nov 29, 2021
@washere
Copy link
Author

washere commented Nov 29, 2021

That is great news, to replicate the issue.
The proposed solution would be the most sound and best strategy too,
Thanks very much.

@Jaifroid
Copy link
Member

Just to document my findings so far:

  • window.devicePixelRatio unfortunately doesn't change until either the app is re-loaded (this can be done programmatically with window.reload() or the window is zoomed with Ctrl + or Ctrl - . This is undoubtedly the source of the error: the browser thinks it is still working with the old screen ratio, but the app registers a slight resize when dragging the window to a new display, and recalculates everything, but unfortunately the browser is giving the wrong values for the derived pixels.
  • After window.reload(), the browser receives the new devicePixelRatio, and hence is able to re-display the article and app chrome with the correct dimensions.
  • As mentioned, the window.resize event does fire on moving the app to a new display. We could tag along a window.reload(), and then the app would display itself correctly; however, this would be very annoying, as there is no way for the app to distinguish between a wanted window resize and this moving to a new screen, hence we would end up reloading the app every time window dimensions are changed.
  • Ironically, if we suppress the app's attempts to recalculate its dimensions on being dragged to a new screen, we would also achieve a correct display. Again, however, we cannot currently tell the difference between a window resize (wanted) and this, and the user would be unable to resize their window until the app has reloaded.
  • I still could try registering the matchMedia event listener (https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio) because it is possible this still fires even though window.devicePixelRatio does not change. If so, we would have a way to register the drag to a new screen, and then we could issue a reload. I'll try this tomorrow if I get time.

@Jaifroid
Copy link
Member

Quick update: unfortunately, registering an event listener on the media style query does not fire when the resolution changes. This is clearly a bug in the framework, but UWP with JavaScript will not now be patched or fixed by Microsoft.

I've also tried to dispatch a keyboard event with Ctrl + followed by Ctrl -, but unfortunately it seems this is a privileged user action that cannot be invoked programmatically.

So this leaves the reload option. I've tested this, and it works, though it's something of a sledgehammer to crack a nut. Clearly it should be limited to the UWP app, and it should be carefully controlled.

@Jaifroid
Copy link
Member

Code so far is in #215.

@washere
Copy link
Author

washere commented Nov 30, 2021

Thanks for your good work on this Jaifroid.
Sometimes sleeping on a bug helps me find the solution next day. I am sure you will find a creative way around.

I just noticed a new way in UI/UX around this apart from CTRL _+ and relaunch on my main laptop screen first.

If i:

  • Drag the app window by top window_bar from external monitor (larger res) to laptop screen
  • Bug goes away & corrects itself
  • Either let go of drag, Or Wait a second or two or three (not too fast) while still dragging
  • Drag back to external monitor

Then it retains the correct aspect.
I don't know if it of any use but thought to report it. It is the fastest way now for me.
Hope it helps others too, will report any new thing if i encounter.

Thanks again for your great work.

@Jaifroid Jaifroid added fixed Fixed, awaiting publication in new apps waiting for feedback labels Dec 1, 2021
@Jaifroid
Copy link
Member

Jaifroid commented Dec 1, 2021

Please see #215 for a package with the proposed fix in it (for testing).

@washere
Copy link
Author

washere commented Dec 1, 2021

Hi Jaifroid,
Thanks for your work. I tested this, initially would not install but turned off firewalls & VPN & re-downloaded & installed.

What happened:

  1. Initially it works fine.
    I can see the bug on launch but it quickly resets to correct zoom out and this process can be seen in about a second.

  2. After some more test:
    Dragging from laptop to monitor screen: it suddenly zoomed in & the bug was back.
    This happens rarely but it does still.
    If last exit was on laptop screen & relaunch & drag to monitor then it happens more.

  3. After more testing (pinned to taskbar) launching from external monitor:
    The bug returns after several relaunches (over 5).
    Then the bug occurs about half the time (about 40% to 50% of relaunches).
    This is when last app exit is on monitor & relaunch is on monitor automatically too.

So we are getting there.
I think it might need some sort of procedure to check after zoom_reset on launch to actually check.
Failing that, maybe a persistent check loop. I will test any further test builds you post in the other thread.

I'll keep retesting this and will report anything new.

Thanks again for your wonderful efforts.

@Jaifroid
Copy link
Member

Jaifroid commented Dec 1, 2021

Thank you very much for the testing @washere. If you could take a look at the release page:
https://github.com/kiwix/kiwix-js-windows/releases/tag/v1.8.1-dev
you can see that I updated the package on 2021-12-1 09:55 UCT . If you downloaded before that, could I ask you to test the new package? You'd need to uninstall, download, and reinstall. I made the screen reset feature more robust, so that it should force the reset on each launch. If it's still not robust enough, there are things I can do. If you downloaded and tested after 2021-12-1 09:55 UCT, then let me know that too. Many thanks.

@washere
Copy link
Author

washere commented Dec 1, 2021

I will test now and report.

@washere
Copy link
Author

washere commented Dec 1, 2021

I uninstalled, redownloaded and installed. And tested this and it is the same file.
Actually it gives the bug most of the time now.

I renamed the first and new download and ran fc in a dosbox:

C:\Tempo>fc 1.exe 2.exe Comparing files 1.exe and 2.EXE FC: no differences encountered

So initial test was on last build too.
In fact the bug happens almost al the time now if last exit & relaunch is from monitor screen.
Why an element of time makes it more return, maybe some temp files or just app cache.
I will test any further builds when i return to the laptop and check for new builds.

Thanks very much.

@washere
Copy link
Author

washere commented Dec 1, 2021

After some time, it happens less, about 30% of relaunches.

@Jaifroid
Copy link
Member

Jaifroid commented Dec 1, 2021

OK, thanks. They shouldn't be .exe files. These are .appxbundles, a very different installation technology. Any .exe inside the bundle is merely the framework, so that will always be identical.

I've got some ideas for making the reset process happen much earlier, and always. And hopefully that will also be much faster to load.

Thank you for the testing. I'll have another ago later today but will let you know if there's a new bundle (and best if I re-number it too).

@washere
Copy link
Author

washere commented Dec 1, 2021

Yes i know, i just renamed to something short so i could type out the fc command fast. i did not install as .exe but as orig filenames.

I will check later when i get back to laptop for any new builds. It is getting there.

Thanks again, have a great day.

@Jaifroid
Copy link
Member

Jaifroid commented Dec 3, 2021

@washere No rush at all, but did you see there is a new package to test?
https://github.com/kiwix/kiwix-js-windows/releases/download/v1.8.1-dev/KiwixWebApp_1.8.11.0_AnyCPU.appxbundle

@washere
Copy link
Author

washere commented Dec 4, 2021

Hi Jaifroid,
Thanks for your work.
I tried the new build with:

  • over 70 relaunches (on external monitor)
  • many option changes in settings

The bug only occurred once.
I think it is good to release as a new version, i think it is better than an RC level.
And it is better than current release.
It is much faster than previous test release 1.18.10.0 and no initial animations.

I think you did a great job.
I wish the android version devs were as good. It seems to hang on library load of external ZIM files, scanning for files. And the large downloads by app itself are hidden & not accessible if device is not rooted. Complaints seem to be about 2 years old and they do not fix anything, many complaints on Google Play store too. I mention that because i wish all devs were as good as you in being actually capable and responsive.

It is much appreciated. So thank you very much in deed.

You can close this issue whenever you want. I consider it fixed.

THANKS AGAIN FOR YOUR GREAT WORK.

@Jaifroid
Copy link
Member

Jaifroid commented Dec 4, 2021

@washere Thank you very much for testing this and confirming it works most of the time.

I've merged this version into master, and will be in a release I hope to do this weekend. Thanks for your kind comments!

@washere
Copy link
Author

washere commented Dec 6, 2021

Hi,
Continued discussion from:
#214

Yes i know size is irrelevant, i wanted to pinpoint the exact file by size as there are several eg(glish) wiktionary files. I also said size is not the issue and smaller nopic wiktionary files should be similar and gave the ftp links to them.

Yesterday it was giving me about a quarter to third of relaunches with the issue.
Today it's better. Relaunched about 65 times and had 6 cases of the issue, so just under 10%.
The relaunch should appear on the external monitor to replicate (should have exited there on last session).

recur

There is one new interesting change that fixes the issue that didn't before the last fix:

  • Double_Click window top_bar to maximize window
  • Issue is fixed, even after double_clicking it again to make the window smaller

Finally I would say, specially in the light of this new fast fix and low occurrence, i consider the issue fixed.
I am sure percentage of recurrence depends on not being wifi extended display, diff resolutions, diff makes & specs of monitors & laptops & current systems (no 2 alike.)

I would say the issue is still fixed and the return from more of your time is not worth it. If the percentage of the issue grows substantially, i will report. But in addition to a quick drag to other display & now double click top-bar & low occurrence, i think this should stay closed & marked as solved.

Of interest: I never had this issue with homepages of zim files that were image heavy, multi columns of images. Also happened more with text only homepages with a long unwrapped text line.

Thanks again & have a great week.

@Jaifroid
Copy link
Member

Jaifroid commented Dec 6, 2021

OK, thanks for the explanation! The reason minimizing and maximizing fixes it is because any resize of the window causes a display reset. This is the only way I currently have of testing that the device has moved to a different monitor. Therefore, the fix (which is a bit of a kludge) issues a reset as soon as it detects any resize of the app's window. In fact, just a very slight resize of the app window should also fix it.

My guess is that the app isn't always detecting the shift from the internal to the external display in time, or else that the reset is happening just a fraction too soon, before the app has actually moved to the new monitor. This would account for why it's happening a bit randomly, albeit rarely. It's probably a race condition.

OK, let's monitor this, and if it becomes annoying or you find a ZIM that fails more regularly, don't hesitate to reopen this issue (or start a new one).

There is a regression I need to fix: #218.

@washere
Copy link
Author

washere commented Dec 6, 2021

Thanks bro,
Yes that explanation would makes sense, on the whole the watching loop seems to be adequately attentive on relaunch & catches it.
Also this Max_window fix only appeared in the last build as far as i can tell, good news.
Thanks again.

Jaifroid added a commit that referenced this issue Apr 24, 2022
Jaifroid added a commit that referenced this issue May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fixed Fixed, awaiting publication in new apps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants