Opened 10 years ago

Closed 10 years ago

#4730 closed bug (fixed)

ShowImage doesn't stop at first/last image

Reported by: idefix Owned by: axeld
Priority: normal Milestone: R1
Component: Applications/ShowImage Version: R1/alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

When you have a couple of images in a folder, named:

1a
12
b
c

You'll notice that when you cycle through them with ShowImage, it will stop at the last image (c), but it alternates between the first (1a) and second (12).

When you rename the first image to 1, ShowImage will also stop at the first image.

I think this bug is caused by tracker's [changeset:29845 natural sorting algorithm], where one program (ShowImage) thinks that 12 comes before 1a and the other (Tracker) thinks 12 comes after 1a.

Attachments (1)

showimage.patch (3.2 KB ) - added by idefix 10 years ago.
[patch] remove ShowImageView::_FindNextImageByDir

Download all attachments as: .zip

Change History (16)

comment:1 by jonas.kirilla, 10 years ago

Anyone opposed to removing ShowImageView::_FindNextImageByDir() http://haiku.it.su.se:8180/source/xref/src/apps/showimage/ShowImageView.cpp#2214 and relying only on Tracker scripting?

Essentially offering Next/Previous image functionality (Alt-Up/Down) only when opened directly from Tracker.

comment:2 by axeld, 10 years ago

Sounds good to me!

in reply to:  1 comment:3 by leavengood, 10 years ago

Replying to jonas.kirilla:

Essentially offering Next/Previous image functionality (Alt-Up/Down) only when opened directly from Tracker.

I don't like this idea. I think there may be potentially several different valid ways to open ShowImage that don't involve Tracker. The Terminal, someone launching ShowImage and then opening an image in a folder, another application opening ShowImage (like the browser in the future maybe.) Losing functionality (not being able to move between images in the same folder) because of this seems inconsistent and not user friendly.

What would be better would to provide a way for Tracker's natural sorting and folder iteration code to be used inside ShowImage, instead of badly trying to emulate it. Like maybe a private API exposed from libtracker.so.

I would say this is one area of many where ShowImage could be refactored because it is duplicating code and functionality. Another that comes to mind is the bitmap smooth scaling, which can now be done efficiently in the app_server.

in reply to:  1 comment:4 by idefix, 10 years ago

Replying to jonas.kirilla:

Anyone opposed to removing ShowImageView::_FindNextImageByDir() http://haiku.it.su.se:8180/source/xref/src/apps/showimage/ShowImageView.cpp#2214 and relying only on Tracker scripting?

Just tested it here and removing that function fixes this bug. It also fixes ticket:4668.

comment:5 by jonas.kirilla, 10 years ago

From a spatial-Tracker point-of-view, I would think it crucial to have applications like Mail and ShowImage traverse the files in Tracker-order. (Unless the app has got a list of its own, separate from Tracker, like a MediaPlayer playlist.)

Tracker order is not necessarily the files merely sorted (natural or not). Up to two columns can influence the sorting in list view mode, currently. Who knows what other view modes we will come up with. (Wondering in what order icon mode folders are best traversed.. Maybe top-left to bottom-right, depending on one's locale?)

Offline (closed) folder entry traversion should be done perfectly (which as you say, might need some Tracker cooperation), or not at all, IMO. I don't really mind either way, but I oppose the imperfection in-between.

comment:6 by jonas.kirilla, 10 years ago

That was supposed to be a reply to leavengood. Oh well. :P

comment:7 by leavengood, 10 years ago

I definitely want the traversal done consistently too. But I think we are dealing with a bit of an edge case here, and I don't want to lose valid functionality over an edge case.

But I think we can solve this in a nice way with a bit of code sharing. I don't know how nice the Tracker folder traversal code is, but if it ugly we can try to refactor it and expose it nicely in a private header for use by ShowImage and whoever else. The fact that most of Tracker's code is already in libtracker.so just makes it that much easier to share.

comment:8 by axeld, 10 years ago

I don't think there really is a lot of functionality lost if we remove that feature (well, I never used it myself, at least). And why should ShowImage behave differently from Mail in that regard? That is inconsistent, at least.

If you don't open ShowImage from Tracker, you simply cannot know the context the image was shown in, so you should not make any assumptions, IMO.

comment:9 by axeld, 10 years ago

Furthermore, Haiku promotes a Tracker centric work flow anyway, and that will get much more powerful with future Tracker enhancements, too.

comment:10 by leavengood, 10 years ago

Honestly I might be grasping at straws trying to find instances where ShowImage was used outside Tracker. Certainly when testing it I would start it myself most of the time. But probably on day to day use it wouldn't be used that way.

I just don't like to see functionality lost.

But if someone wants to make this change, go ahead. If in further testing it does seem like a loss of functionality we can then investigate other options.

in reply to:  10 comment:11 by jonas.kirilla, 10 years ago

Replying to leavengood:

instances where ShowImage was used outside Tracker.

The most prominent one must be opening ShowImage indirectly via Deskbar's Recent Documents list.

comment:12 by leavengood, 10 years ago

OK, so I talked with Rene and have been more convinced about removing the directory iteration stuff. Plus it helps that it fixes a couple bugs :)

Also we talked about the cases where you could still correctly iterate over multiple images when no Tracker is involved, namely:

  • If multiple images are opened from the Terminal, treat those as a list to be iterated over in the order given, instead of opening separate windows, which is what happens now.
  • Allow the BFilePanel used for opening images to be multi-select, and treat the multiple selections as a list to iterate over.

I think these changes would be nice and would allow for the best of both worlds.

As for Recent Documents, I guess those will just have to be treated as single images.

in reply to:  12 comment:13 by jonas.kirilla, 10 years ago

Replying to leavengood:

cases where you could still correctly iterate over multiple images when no Tracker is involved

As ArgvReceived() calls RefsReceived(), one could (with a few changes to ShowImage) propagate a B_REFS_RECIVED message all the way to the window (or its view, iirc), letting it decide for itself how to enable iteration, based on the message content. (Multiple refs, presence of a messenger, ?..) The window could keep the message and iterate over the contained refs, if multiple.

(Watch out for this though: Currently Tracker's primary means to open (plain "Open") sends refs as individual messages though, which I think should be fixed. "Open With" sends entry_refs as a multi-refs BMessage.)

by idefix, 10 years ago

Attachment: showimage.patch added

[patch] remove ShowImageView::_FindNextImageByDir

comment:14 by axeld, 10 years ago

Owner: changed from leavengood to axeld
Status: newassigned

comment:15 by axeld, 10 years ago

Resolution: fixed
Status: assignedclosed

Thanks for the fix, applied in hrev34103.

Note: See TracTickets for help on using tickets.