Bug 429447 - URL Navigator has only half of the available width
Summary: URL Navigator has only half of the available width
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: general (show other bugs)
Version: 20.11.80
Platform: openSUSE Linux
: NOR normal
Target Milestone: ---
Assignee: Felix Ernst
URL:
Keywords:
: 430769 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-11-21 12:57 UTC by Fabian Vogt
Modified: 2021-01-06 01:38 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 21.04.0
Sentry Crash Report:


Attachments
Preliminary Patch (8.37 KB, patch)
2020-11-27 18:04 UTC, Felix Ernst
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Vogt 2020-11-21 12:57:25 UTC
Found by openQA: https://openqa.opensuse.org/tests/1480197#step/dolphin/5

When clicking on that, the input field is also just half of the available size: https://i.imgur.com/vJyMeDU.png
Comment 1 Fabian Vogt 2020-11-21 12:59:19 UTC
What should be noted is that dolphin here has an additional toolbar button by default:

Index: dolphin-19.03.60git.20201029T150039~bf11c835e/src/dolphinui.rc
===================================================================
--- dolphin-19.03.60git.20201029T150039~bf11c835e.orig/src/dolphinui.rc	2020-10-29 22:00:39.000000000 +0100
+++ dolphin-19.03.60git.20201029T150039~bf11c835e/src/dolphinui.rc	2020-10-31 09:57:20.003032608 +0100
@@ -115,6 +115,7 @@
         <text context="@title:menu">Main Toolbar</text>
         <Action name="go_back" />
         <Action name="go_forward" />
+        <Action name="go_up" />
         <Separator name="separator_1" />
         <Action name="icons" />
         <Action name="compact" />

Maybe that's what triggers this bug.
Comment 2 Felix Ernst 2020-11-23 14:14:39 UTC
I cannot reproduce this. I am also not familiar with opensuse (I mean I know what it is but never used it; greetings to Nürnberg) or openQA. Was this bug ever confirmed by a human? Might there be an expanding spacer to the left of the URL Navigator? Can it be that the picture was taken in the 100 ms time frame Dolphin sometimes waits before adjusting the spacing? Does the problem persist after resizing Dolphin once? Is the dolphinui.rc file aside from the "up" button identical to the one from master?
Comment 3 Fabian Vogt 2020-11-23 14:26:06 UTC
(In reply to Felix Ernst from comment #2)
> I cannot reproduce this. I am also not familiar with opensuse (I mean I know
> what it is but never used it; greetings to Nürnberg) or openQA. Was this bug
> ever confirmed by a human?

Yes, I can reproduce it.

> Might there be an expanding spacer to the left of
> the URL Navigator? Can it be that the picture was taken in the 100 ms time
> frame Dolphin sometimes waits before adjusting the spacing?

No, I can interact with the window normally and it stays that way, no matter how
long I wait.

> Does the problem
> persist after resizing Dolphin once?

After changing the width or height by just 1px it snaps to the right position and
size indeed.

> Is the dolphinui.rc file aside from the
> "up" button identical to the one from master?

Yes.
Comment 4 Felix Ernst 2020-11-23 15:02:31 UTC
> After changing the width or height by just 1px it snaps
> to the right position and size indeed.

And the next time you start Dolphin the URL Navigator is at the wrong position again?

Does clicking on a folder also make it snap to the right position?
Comment 5 Felix Ernst 2020-11-23 15:03:46 UTC
> Does clicking on a folder also make it snap to the right position?

I mean changing the viewed folder.
Comment 6 Fabian Vogt 2020-11-23 15:05:15 UTC
(In reply to Felix Ernst from comment #4)
> > After changing the width or height by just 1px it snaps
> > to the right position and size indeed.
> 
> And the next time you start Dolphin the URL Navigator is at the wrong
> position again?

It seems like only the first start is affected, even just opening dolphin and immediately closing it again makes it work on the next start.

> Does clicking on a folder also make it snap to the right position?

No, that doesn't seem to make any difference.
Comment 7 Nate Graham 2020-11-23 15:56:42 UTC
I can't reproduce this either FWIW.
Comment 8 Felix Ernst 2020-11-23 16:23:34 UTC
(In reply to Fabian Vogt from comment #6)
> It seems like only the first start is affected, even just
> opening dolphin and immediately closing it again makes
> it work on the next start.

Okay, that's good. But unfortunately this also means that debugging this touches on a lot of subjects I know very little of.

I'll write down what I know about this now so people more familiar with Dolphin startup, session restoration or Qt Application startup have an easy time resolving this bug report if they are able to reproduce it.

- The calculation of the spacing done by DolphinNavigatorsWidgetAction::adjustSpacing() is triggered whenever a viewContainer is resized or an url is changed. Because changing the folder and therefore the url didn't fix the spacing for Fabain Vogt that means that adjustSpacing() is calculating the spacing based on wrong cached geometry of the viewContainers to begin with.

- Geometry of the viewContainers is cached whenever DolphinNavigatorsWidgetAction::followViewContainerGeometry() is called. This happens only from DolphinTabPage::eventFilter() whenever a QEvent::Resize occurs for a viewContainer. This is why resizing Dolphin fixes the spacing.

- This bug occuring on the first startup therefore signifies to me that there are either resize events occuring at a time at which Qt doesn't report correct geometries yet or that no resize events are occuring at all. Whenever I start Dolphin with a single tab and a single view, a total of three calls to DolphinNavigatorsWidgetAction::followViewContainerGeometry() happen, all of which leading to the correct leading spacing of 0.

That's all I know for now.

(In reply to Nate Graham from comment #7)
> I can't reproduce this either FWIW.

I was hoping your session restoration knowledge could come in handy here. :P
Comment 9 Felix Ernst 2020-11-23 16:24:58 UTC
I hope this doesn't happen for all new installations.
Comment 10 Fabian Vogt 2020-11-23 16:57:18 UTC
I see that the code is using mapToGlobal quite liberally. Note that mapToGlobal really uses global screen coordinates, which aren't reliable and shouldn't be used for such calculations.

I set a breakpoint at followViewContainerGeometry and it's called even before the window is visible, so mapToGlobal will return wrong values as there's no window yet. That might be the cause of this.

It probably works for resizes because the window is visible at that point and the stored window size might just trigger a resize event with similar effect.
Comment 11 Fabian Vogt 2020-11-23 18:50:16 UTC
(In reply to Fabian Vogt from comment #10)
> I see that the code is using mapToGlobal quite liberally. Note that
> mapToGlobal really uses global screen coordinates, which aren't reliable and
> shouldn't be used for such calculations.
> 
> I set a breakpoint at followViewContainerGeometry and it's called even
> before the window is visible, so mapToGlobal will return wrong values as
> there's no window yet. That might be the cause of this.
> 
> It probably works for resizes because the window is visible at that point
> and the stored window size might just trigger a resize event with similar
> effect.

This seems to be the case indeed. For the first few calls of adjustSpacing, m_globalXOfSplitter is 0 here as it's just not shown yet. By updating m_globalXOfSplitter in adjustSpacing, it's set propely on the fourth call and the spacing calculations are correct.
Comment 12 Fabian Vogt 2020-11-26 19:49:58 UTC
(In reply to Fabian Vogt from comment #11)
> (In reply to Fabian Vogt from comment #10)
> > I see that the code is using mapToGlobal quite liberally. Note that
> > mapToGlobal really uses global screen coordinates, which aren't reliable and
> > shouldn't be used for such calculations.
> > 
> > I set a breakpoint at followViewContainerGeometry and it's called even
> > before the window is visible, so mapToGlobal will return wrong values as
> > there's no window yet. That might be the cause of this.
> > 
> > It probably works for resizes because the window is visible at that point
> > and the stored window size might just trigger a resize event with similar
> > effect.
> 
> This seems to be the case indeed. For the first few calls of adjustSpacing,
> m_globalXOfSplitter is 0 here as it's just not shown yet. By updating
> m_globalXOfSplitter in adjustSpacing, it's set propely on the fourth call
> and the spacing calculations are correct.

Should I just submit ^ as workaround?

In openQA this hits in 100% of all first starts, so I expect that users will
notice this. It's not that bad as just resizing the window or restarting
dolphin fixes it, but it's literally the first impression...
Comment 13 Felix Ernst 2020-11-27 18:04:10 UTC
Created attachment 133691 [details]
Preliminary Patch

(In reply to Fabian Vogt from comment #12)
> (In reply to Fabian Vogt from comment #11)
> > By updating
> > m_globalXOfSplitter in adjustSpacing, it's set propely on the fourth call
> > and the spacing calculations are correct.
> 
> Should I just submit ^ as workaround?

Unfortunately moving just the calculation of m_globalXOfSplitter into adjustSpacing() will introduce a different bug I ran into already: All other values are cached whenever Dolphin is resized but m_globalXOfSplitter would then change whenever adjustSpacing() is calculated, e.g. when changing the folder. Moving the window and then changing the folder would then lead to wrong spacing because m_globalXOfSplitter won't have the correct value relative to the others.

I created a patch that completely updates all cached geometry every time adjustSpacing() is called. Because that happens once on a timer 100 ms after every url change, it will happen once shortly after the window is shown. I am hoping at that point all geometry is where it should be.

Could you see if the attached patch fixes the problem?
Comment 14 Fabian Vogt 2020-11-27 19:07:36 UTC
(In reply to Felix Ernst from comment #13)
> Created attachment 133691 [details]
> Preliminary Patch
> 
> (In reply to Fabian Vogt from comment #12)
> > (In reply to Fabian Vogt from comment #11)
> > > By updating
> > > m_globalXOfSplitter in adjustSpacing, it's set propely on the fourth call
> > > and the spacing calculations are correct.
> > 
> > Should I just submit ^ as workaround?
> 
> Unfortunately moving just the calculation of m_globalXOfSplitter into
> adjustSpacing() will introduce a different bug I ran into already: All other
> values are cached whenever Dolphin is resized but m_globalXOfSplitter would
> then change whenever adjustSpacing() is calculated, e.g. when changing the
> folder. Moving the window and then changing the folder would then lead to
> wrong spacing because m_globalXOfSplitter won't have the correct value
> relative to the others.
>
> I created a patch that completely updates all cached geometry every time
> adjustSpacing() is called. Because that happens once on a timer 100 ms after
> every url change, it will happen once shortly after the window is shown. I
> am hoping at that point all geometry is where it should be.
> 
> Could you see if the attached patch fixes the problem?

Yep, that works!
Comment 15 Nate Graham 2020-11-28 19:32:39 UTC
Please feel free to submit that as a merge request! :)
Comment 16 Felix Ernst 2020-11-30 13:41:37 UTC
I'll do that. I only want to clean it up a bit first.
Comment 17 Nate Graham 2020-11-30 14:25:15 UTC
Sounds good, thanks!

Keep in mind that final tagging for 20.12 is in 3 days: https://community.kde.org/Schedules/release_service/20.12_Release_Schedule#Thursday.2C_December_3.2C_2020:_release_service_20.12_Tagging
Comment 18 Fabian Vogt 2020-12-06 10:56:18 UTC
(In reply to Nate Graham from comment #17)
> Sounds good, thanks!
> 
> Keep in mind that final tagging for 20.12 is in 3 days:
> https://community.kde.org/Schedules/release_service/20.
> 12_Release_Schedule#Thursday.2C_December_3.2C_2020:_release_service_20.
> 12_Tagging

It seems like the deadline was missed?
Comment 19 Felix Ernst 2020-12-11 10:50:06 UTC
Yes. I think that's okay in this case. Unfortunately I am quite busy at the moment. Trust me when I say that I would rather contribute to KDE than dealing with what I have to deal with at the moment. I'll be able to create and deal with a MR within a month I think.
Comment 20 Bug Janitor Service 2020-12-16 13:46:37 UTC
A possibly relevant merge request was started @ https://invent.kde.org/system/dolphin/-/merge_requests/144
Comment 21 Nate Graham 2020-12-28 21:17:57 UTC
*** Bug 430769 has been marked as a duplicate of this bug. ***
Comment 22 Felix Ernst 2021-01-06 01:38:51 UTC
Git commit 0cee94ce82ccb82afd0c4e22d77e251276e7a447 by Felix Ernst.
Committed on 06/01/2021 at 01:38.
Pushed by felixernst into branch 'master'.

Fix location bar being wrongly aligned on first startup

When starting Dolphin the very first time, the spacing in front of the
location bar is wrong. This commit fixes this by completely updating
all cached geometry every time adjustSpacing() is called. Because this
happens once on a timer 100 ms after every url change, it will happen
once shortly after the window is shown. At that point all geometry is
where it should be and spacing calculation works as expected.

The ViewContainer geometry retrieval is refactored into a small nested
helper class in DolphinNavigatorsWidgetAction by the name
ViewGeometriesHelper.

Previously the logic of that class was divided between DolphinTabPage
and DolphinNavigatorsWidgetAction.
FIXED-IN: 21.04.0

M  +128  -88   src/dolphinnavigatorswidgetaction.cpp
M  +58   -20   src/dolphinnavigatorswidgetaction.h
M  +5    -29   src/dolphintabpage.cpp
M  +0    -5    src/dolphintabpage.h

https://invent.kde.org/system/dolphin/commit/0cee94ce82ccb82afd0c4e22d77e251276e7a447