Closed Bug 233763 Opened 21 years ago Closed 21 years ago

Delete Dialog 64 char bug

Categories

(SeaMonkey :: Installer, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: opi, Assigned: opi)

References

Details

Attachments

(2 files, 4 obsolete files)

If in config.ini on section [LegacyCheck0] you have a message which have a space on a 64 char, the installer chows the same content on all following lines. Maybe another bug is, that on lines with 64 chars the next chars aren't printed to the Dialog. An example is the from me mentioned new string in #69153
Attached patch patch diff -u8p (trunk) (obsolete) — Splinter Review
Seems easiest patch to a very strang if condition
Comment on attachment 141115 [details] [diff] [review] patch diff -u8p (trunk) > msgChunk[msgChunkPtr - msgChunk] = 0; wouldn't this be happier as *msgChunkPtr = '\0'; >- msgPtr = msgPtr + (msgChunkPtr - msgChunk + 1); >+ msgPtr += (msgChunkPtr - msgChunk + 1); >+ } else >+ { >+ msgPtr += 64; fix indenting (tabs?) also the formatting style used in the linux installer is: code; } else { code;
Attached patch patch v2 (obsolete) — Splinter Review
> fix indenting (tabs?) OK, done >also the formatting style ... OK, done ... I forgett it. >wouldn't this be happier as ... Yes, looks nicer ... the line wasn't in question yesterday. Thanks Andrew
Attachment #141115 - Attachment is obsolete: true
Attachment #141144 - Flags: review?(bsmedberg)
Attachment #141144 - Flags: approval1.7a?
deveditz, can you super review?
Attachment #141144 - Flags: superreview?(dveditz+bmo)
Comment on attachment 141144 [details] [diff] [review] patch v2 I'd prefer cleaning up the routine as long as we're touching it (e.g. replace magic numbers with consts or sizeof, set numlines to min(numlines,20) rather than double testing each time through the loop, move the constant strlen() out of the loop) but it's not strictly necessary and might get in the way of the attempt to slip this fix into 1.7a In addition to the mentioned symptoms, if a line didn't have any spaces the old code would write nulls to address 00000000 > strncpy(msgChunk, msgPtr, 64); > > // pad by a line but don't allow overflow > if (msgPtr > msg + strlen(msg)) > break; > msgPtr might point to data past the allocated buffer. We break out of the loop before we use junk, but if that part of the heap happens to be allocated to another process we could crash. Move the if...break to the end of the loop after msgPtr is incremented (or add it to the for() condition) rather than checking after it's used. Otherwise looks good.
Attachment #141144 - Flags: superreview?(dveditz+bmo) → superreview-
Attached patch patch v3 (obsolete) — Splinter Review
Daniel I hope I've understand you correct :-) I've removed the numLines, cause it works very stupid if you have many words with much chars and it makes no sence. The strlen(msg) is moved out and the for loop have the msgPtr condition. And I hope I've understand the 'replace magic numbers' correct.
Attachment #141144 - Attachment is obsolete: true
Attachment #141228 - Flags: superreview?(dveditz+bmo)
Attached patch patch v3.1 (obsolete) — Splinter Review
small fix, thx StefanB
Attachment #141228 - Attachment is obsolete: true
Attachment #141230 - Flags: superreview?(dveditz+bmo)
Comment on attachment 141230 [details] [diff] [review] patch v3.1 >- char *msg = NULL, *msgPtr = NULL, *msgChunkPtr = NULL; >- char msgChunk[65]; >+ char msgChunk[MAXCHARS+1]; >+ char msg[1024]; Thanks for the magic number cleanup. msg still has to be a char* because you're getting a pointer to a string in a resource structure. >- // wrap message at 64 columns, and truncate at 20 rows This comment is valuable in explaining what the loop is trying to do. You could replace the raw numbers with your constant names, but we still want a comment here. Ah, I see a later comment about stopping at MAXLINES rows. I still think you want to mention that we're trying to wrap lines, otherwise your new comment raises the question "stop doing WHAT at maxlines?" >+ numChars = strlen(msg); >+ // stop at MAXLINES rows or stop after last char is reached >+ for (i = 0; i < MAXLINES && msgPtr <= msg + numChars; i++) Since you don't use numChars anywhere else it'd be slightly better with a char* msgEnd = msgPtr + strlen(msg), then compare msgPtr and msgEnd in the loop. Ok, so it doesn't *really* help performance here over a max of 20 loops, but if you get in good habits then you'll do the right thing when it does matter. Either way I think you want '<' not '<=' though the extra blank line when they're equal won't kill anyone. Just to be clear, I'm rejecting this patch because of the incorrect "char msg[1024];". The rest expresses my (strong) preferences but are more matters of style.
Attachment #141230 - Flags: superreview?(dveditz+bmo) → superreview-
Attachment #141228 - Flags: superreview?(dveditz+bmo)
Attached patch patch v3.2Splinter Review
Excuse my fault Daniel :-) >+ char msg[1024]; This is from another patch (which is on the blokked bug #69153) > otherwise your new comment raises the question "stop doing WHAT at maxlines?" I'd wrote it as comment in the first lin of the for ... but hope now it is more clean. > with a char* msgEnd = msgPtr + strlen(msg) done > you want '<' not '<=' though done
Attachment #141230 - Attachment is obsolete: true
Attachment #141266 - Flags: superreview?(dveditz+bmo)
Comment on attachment 141266 [details] [diff] [review] patch v3.2 >+ msgPtr += 64; The MAXCHARS changed back into a raw number again. I trust you to fix that when you check in. I'll count Andrew's earlier review as the r= and give you r/sr=dveditz Who is going to check in? I don't have access to a Linux machine right now and I don't feel good merging in patches without being able to build and test them.
Attachment #141266 - Flags: superreview?(dveditz+bmo)
Attachment #141266 - Flags: superreview+
Attachment #141266 - Flags: review+
Attachment #141266 - Flags: approval1.7a+
a=dveditz for 1.7a
@Daniel Thank you very much for all and excuse me little broken patches I'm a bit nervous at the moment.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
fix checked in by timeless
Attachment #141144 - Flags: review?(bsmedberg)
Attachment #141144 - Flags: approval1.7a?
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: