Bug 94726 - Ksnapshot counter can't count over 9 anymore
Summary: Ksnapshot counter can't count over 9 anymore
Status: RESOLVED FIXED
Alias: None
Product: ksnapshot
Classification: Applications
Component: general (show other bugs)
Version: 0.7
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Richard Moore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-12-09 10:14 UTC by David de Beer
Modified: 2005-03-17 08:01 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch fixing bug and improving extension behaviour (2.71 KB, patch)
2004-12-11 23:20 UTC, Philippe Rigault
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David de Beer 2004-12-09 10:14:29 UTC
Version:           0.7 (using KDE 3.3.0, SuSE)
Compiler:          gcc version 3.3.4 (pre 3.3.5 20040809)
OS:                Linux (i686) release 2.6.8-24.5-default

When taking large amounts of snapshots for step by step manuals for example, the newer version of Ksnaphot that comes with SuSE 9.2 KDE 3.3 can't count over nine.
one only notices after 19, then the next number will be 110,111 aso 119,1110  instead of 30.
The previous version of ksnapshot simply counted on and on, so sorting order could well be kept.

This is a really annoying, and on renaming / resorting time consuming bug.

Please some change to it, or advise how to change it if it can be done outside the code.
Comment 1 Philippe Rigault 2004-12-11 18:56:03 UTC
Confirmed
KDE-3.3.2 compiled from sources (gcc-3.3.3 on Fedora Core 1).
Comment 2 Philippe Rigault 2004-12-11 19:36:01 UTC
Looking into it. The bug is in
void KSnapshot::autoincFilename()
Expect a patch soon.
Comment 3 Philippe Rigault 2004-12-11 23:20:12 UTC
Created attachment 8627 [details]
Patch fixing bug and improving extension behaviour

This patch does two things:

1. Fixes the bug (i.e number is always incremented by one). I got rid of
QRegExp because it looks like QRegExp::searchRev() has a bug, i.e:

    QString name = "foo123.png";
    QRegExp rx("[0-9]+");
    int i = rx.searchRev(name); //i == 5 instead of 3 

I will ask the trolls about this.

2. Deals with extensions in a better way, by making sure that when it looks for
a number in the filename, the extension is not considered. This way, extensions
that contain a number (i.e mp3) won't get modified.

The behaviour for incrementation is now:
foo -> foo1
foo1 -> foo2
foo19 -> foo20
foo.png -> foo1.png
foo1.png -> foo2.png
foo.mp3 -> foo1.mp3
foo1.mp3 -> foo2.mp3
foo. -> foo1.
foo1. -> foo2.
.png -> 1.png
.mp3 -> 1.mp3

Note to maintainers: I left the debug statements (commented out for the release
version) in the code so that you can have a feel of the exact algorithm by
uncommenting them.

Please commit.
Comment 4 Martin Koller 2005-01-21 21:47:04 UTC
Current CVS does no longer show this problem
Comment 5 Philippe Rigault 2005-03-02 04:28:34 UTC
>Current CVS does no longer show this problem 

Yes it does. And 3.4.0-rc1 too.

Please reopen, it is NOT fixed.

The behaviour mentioned in comment #2 is not a bug in the QRegExp::searchRev() function (as confirmed by trolltech), it is by design, which is worse.
Please read http://bugs.kde.org/show_bug.cgi?id=100107#c9 to see exactly what  QRegExp::searchRev() does, and why it is a bogus function.

It is most certainly not what the author of ksnapshot.cpp had in mind, given that calling in reality searchRev() used with QRegExp rx("[0-9]+") is _guaranteed_ to _never_ match more than one digit (the last one).

I reiterate my call to commit my patch above, and to avoid using 
QRegExp::searchRev() altogether.

regards,

Philippe
Comment 6 David de Beer 2005-03-04 16:16:10 UTC
reopened as requested
Comment 7 Richard Moore 2005-03-13 01:09:19 UTC
CVS commit by rich: 

Revert back to the orignal 'first number' behaviour of the filename increment.

BUG: 94726


  M +1 -1      ksnapshot.cpp   1.80


--- kdegraphics/ksnapshot/ksnapshot.cpp  #1.79:1.80
@@ -378,5 +378,5 @@ void KSnapshot::autoincFilename()
 
     // Does it have a number?
-    int start = numSearch.searchRev(name);
+    int start = numSearch.search(name);
     if (start != -1) {
         // It has a number, increment it
Comment 8 Philippe Rigault 2005-03-14 16:19:17 UTC
This fixes the 19->100 bug, but reverts to substandard 'first number' behaviour, with all its pitfalls including ignoring extensions.

You will get this kind of behaviour now:

up2date_menu_1.png -> up3date_menu_1.png
xorg-x11-devel_config_1.png -> xorg-x12-devel_config_1.png 
foo.mp3 -> foo.mp4

which not really satisfactory.

I think your change from 'first number' to 'last number' was a change for the good in terms of interface, so why not fix the way it was implemented and get it right with extensions at the same time ?

Comment 9 Richard Moore 2005-03-14 22:56:18 UTC
On Mon, Mar 14, 2005 at 03:19:18PM -0000, Philippe Rigault wrote:
> 
> I think your change from 'first number' to 'last number' was a change for the good in terms of interface, so why not fix the way it was implemented and get it right with extensions at the same time ?


Because a correct solution needs the user to be able to configure the
insertion point which will necessarily introduce new translation
strings.

Cheers

Rich.
Comment 10 David de Beer 2005-03-17 08:01:45 UTC
Even if it is not in all points as all would like, a fixed 19>100 bug should work now makes it adaptable, workable for end-users. 
One will have to take care in file naming though, but that is usance where incremental numbers are involved. Any further refinements are appreciated of course, but beyond the intention of my bug-report..
Thanks, and now I hope I will get it implemented somehow..