Display full path of image on main window
Needs ReviewPublic

Authored by steffenh on Jul 23 2018, 6:47 AM.

Details

Reviewers
None
Group Reviewers
Gwenview
Summary

Display the full path of image on the main window and
display the active image name, if you have more than one
image in the view mode.

BUG: 378061

Diff Detail

Repository
R260 Gwenview
Branch
urlincaption
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1071
Build 1084: arc lint + arc unit
steffenh requested review of this revision.Jul 23 2018, 6:47 AM
steffenh created this revision.
steffenh edited the summary of this revision. (Show Details)
rkflx added a subscriber: rkflx.Jul 23 2018, 7:38 AM

Interesting idea ;)

FWIW, I've also been testing patches adding the folder name to the window title in Browse mode, although they work more like in Dolphin (and are a bit more complex, because they also fix some bugs):
P244
P245
P246

Should I bin those now, or submit them as Diffs?


display the active image name, if you have more than one image in the view mode

Do you mean that in Compare mode the window title should change when clicking on another image? For me this is already the case without your patch.

rkflx added a comment.Jul 23 2018, 8:20 AM

Bug 378061

Thanks for adding that to the summary, but in general please make sure to follow the exact syntax, i.e. BUG: 378061 (see https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords), to make sure the infrastructure picks it up correctly.

That said, I'm not sure on how we should handle the bug, see my comment added over there.

steffenh edited the summary of this revision. (Show Details)Jul 23 2018, 9:00 AM
steffenh edited the summary of this revision. (Show Details)

Hi, @rkflx

display the active image name, if you have more than one image in the view mode

Do you mean that in Compare mode the window title should change when clicking on another image? For me this is already the case without your patch.

Interesting, in KDE Neon 5.13 (Plasma 5.13.2, KDE Framework 5.47.0, Qt 5.11.0) yes it works without my patch. In my working Linux (OpenSuse Leap15.0, Plasma 5.12.5, KDE Framework 5.45.0, Qt 5.9.4) is it doesn't work without my patch.

Thanks for adding that to the summary, but in general please make sure to follow the exact syntax, i.e. BUG: 378061 (see https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords), to make sure the infrastructure picks it up correctly.

Thanks for the link, I will follow the syntax in the future.

display the active image name, if you have more than one image in the view mode

Do you mean that in Compare mode the window title should change when clicking on another image? For me this is already the case without your patch.

Interesting, in KDE Neon 5.13 (Plasma 5.13.2, KDE Framework 5.47.0, Qt 5.11.0) yes it works without my patch. In my working Linux (OpenSuse Leap15.0, Plasma 5.12.5, KDE Framework 5.45.0, Qt 5.9.4) is it doesn't work without my patch.

On my Kubuntu 18.04 this works without patch too (Plasma 5.12.6, Frameworks 5.44.0, Qt 5.9.5).
The first Gwenview version backwards it's not working is branch Applications/17.12. (I remember vaguely there was a patch for this sometime.)

display the active image name, if you have more than one image in the view mode

Do you mean that in Compare mode the window title should change when clicking on another image? For me this is already the case without your patch.

Interesting, in KDE Neon 5.13 (Plasma 5.13.2, KDE Framework 5.47.0, Qt 5.11.0) yes it works without my patch. In my working Linux (OpenSuse Leap15.0, Plasma 5.12.5, KDE Framework 5.45.0, Qt 5.9.4) is it doesn't work without my patch.

Neon has Gwenview 18.04.3, while Leap has 17.12.3, the latter of which lacks the fix (a7819f6eb53c, found by reading through this). Are you sure you are running your self-compiled Gwenview on Leap, in particular in such a way that it picks up your self-compiled libgwenviewlib.so?


Interesting idea ;)

FWIW, I've also been testing patches adding the folder name to the window title in Browse mode, although they work more like in Dolphin (and are a bit more complex, because they also fix some bugs):
P244
P245
P246

Should I bin those now, or submit them as Diffs?

Let me detail why I think my approach is preferable:

  • Only displays the folder name, but not the full path (which is not what we want, see bug).
  • Works more like in a stock Dolphin.
  • Does not fail for remote images.
  • Displays protocol and server name for remote images.
  • Displays Place name instead of directory if applicable, consistent with the URL bar.
  • Fixes bugs switching to the Start Page.
  • Does not fail when renaming the directory from Dolphin.
  • Consistent handling of the trailing slash.
  • windowTitle function to ensure a consistent naming even when called from multiple places, or the code is reorganized at a later time.

It's really unfortunate that both of our patches are clashing a bit. What worked really well in the past for us is first opening a task or commenting on an existing bug if you are a new contributor and want to work on something, so we can better guide the design and see how this fits into existing efforts. (I've been meaning to document more plans on the public workboard, sadly not done yet… I know this must be frustrating for you, and I'm sorry for it.)

Let us know if you need ideas for different bugs or features in Gwenview for you to work on!

Hi @rkflx

Neon has Gwenview 18.04.3, while Leap has 17.12.3, the latter of which lacks the fix (a7819f6eb53c, found by reading through this). Are you sure you are running your self-compiled Gwenview on Leap, in particular in such a way that it picks up your self-compiled libgwenviewlib.so?

I not sure, I have used KDevelop 5.2.3 to get the Gwenview source from git. I make a new branch from master to do my coding and use the run or the debug function from KDevelop to test my work. It should pick up my self-compiled library or not?

Let me detail why I think my approach is preferable:

  • Only displays the folder name, but not the full path (which is not what we want, see bug).
  • Works more like in a stock Dolphin.
  • Does not fail for remote images.
  • Displays protocol and server name for remote images.
  • Displays Place name instead of directory if applicable, consistent with the URL bar.
  • Fixes bugs switching to the Start Page.
  • Does not fail when renaming the directory from Dolphin.
  • Consistent handling of the trailing slash.
  • windowTitle function to ensure a consistent naming even when called from multiple places, or the code is reorganized at a later time.

    It's really unfortunate that both of our patches are clashing a bit. What worked really well in the past for us is first opening a task or commenting on an existing bug if you are a new contributor and want to work on something, so we can better guide the design and see how this fits into existing efforts. (I've been meaning to document more plans on the public workboard, sadly not done yet… I know this must be frustrating for you, and I'm sorry for it.)

    Let us know if you need ideas for different bugs or features in Gwenview for you to work on!

I think your work is better, I am new to this and learning by doing at the moment.
I think only to display the folder name is not good enough. I can see a situation, where I have a folder 'Pictures' on a USB stick and also have the folder 'Pictures' in my home, so in this case I can not easily tell apart between two Gwenview windows.

rkflx added a comment.Jul 23 2018, 2:49 PM

I not sure, I have used KDevelop 5.2.3 to get the Gwenview source from git. I make a new branch from master to do my coding and use the run or the debug function from KDevelop to test my work. It should pick up my self-compiled library or not?

I just tried Leap 15, and fetched and built Gwenview from KDevelop (oddly enough, I only had version 5.1.2). I then added a qDebug() << "ping"; to RasterImageView::resizeEvent, which is a source file in lib/. RunDebug Launch would then start Gwenview, with the window title properly changing in Compare mode and the debug output showing as I would expect.

Therefore the most likely explanation is that either you are on the wrong branch or haven't updated your checkout in a while with git pull, or your setup or environment is somehow different to a stock Leap installation.

You could try replicating my test from above and check your branches. If that does not help, I would suggest to try again with a fresh checkout and without KDevelop, and list the exact commands you ran along with any changes to environment variables (let us know if we should help you with doing all that in a shell). You really should be able to get Compare mode working with the normal master branch.

There is also IRC to get help from others, if you prefer that.

I'm sure you'll figure out the issue eventually ;)


I think only to display the folder name is not good enough. I can see a situation, where I have a folder 'Pictures' on a USB stick and also have the folder 'Pictures' in my home, so in this case I can not easily tell apart between two Gwenview windows.

That's correct, but we have to weigh priorities here: Showing the full path would mean displaying something like this in the taskbar:

/home/user/Pictures/… (the rest is cut off)
/home/user/Pictures/… (the rest is cut off)

Only showing the folder would look like this:

Holiday_2018 (not cut off anymore)
Roadtrip_2018 (not cut off anymore)

I'd argue that having multiple windows open where the first few directories are the same is much more common than your example with the same folder name but different mount points (/home, /run/media etc.). The most important information is Pictures, /home/user is less important and therefore due to space constraints in the taskbar and not overloading the UI with too much text in general should not be displayed. Dolphin is doing the same by default, just like pretty much every other file manager out there, where your argument would also apply. As I wrote in the bug, the URL bar already displays the full path, and that should be enough.

If there was a global preference configurable through System Settings to change the style, Gwenview could respect that. However, micro-managing such settings in Gwenview itself is not something we should do, or otherwise the UI would become bloated very fast.

Hi @rkflx

You could try replicating my test from above and check your branches. If that does not help, I would suggest to try again with a fresh checkout and without KDevelop, and list the exact commands you ran along with any changes to environment variables (let us know if we should help you with doing all that in a shell). You really should be able to get Compare mode working with the normal master branch.

Thank you for the help, I have it working now.

I think only to display the folder name is not good enough. I can see a situation, where I have a folder 'Pictures' on a USB stick and also have the folder 'Pictures' in my home, so in this case I can not easily tell apart between two Gwenview windows.

That's correct, but we have to weigh priorities here: Showing the full path would mean displaying something like this in the taskbar:

/home/user/Pictures/… (the rest is cut off)
/home/user/Pictures/… (the rest is cut off)

Only showing the folder would look like this:

Holiday_2018 (not cut off anymore)
Roadtrip_2018 (not cut off anymore)

I'd argue that having multiple windows open where the first few directories are the same is much more common than your example with the same folder name but different mount points (/home, /run/media etc.). The most important information is Pictures, /home/user is less important and therefore due to space constraints in the taskbar and not overloading the UI with too much text in general should not be displayed. Dolphin is doing the same by default, just like pretty much every other file manager out there, where your argument would also apply. As I wrote in the bug, the URL bar already displays the full path, and that should be enough.

If there was a global preference configurable through System Settings to change the style, Gwenview could respect that. However, micro-managing such settings in Gwenview itself is not something we should do, or otherwise the UI would become bloated very fast.

I can see your point with the taskbar, so after same thinking, I think the short path is good enough.

rkflx added a comment.Jul 23 2018, 7:17 PM

Thank you for the help, I have it working now.

Glad to hear!

Any insight you could share, so we can help out future new contributors more efficiently if they also run into trouble, or adapt our documentation?