Bounty proposal: Paint

Argh. I’d run into that one (dragging multiple sprites) but thought it was a limitation that you could only do one at a time, rather than recognising it as a bug.

Rick I just opened about 17 sprite files and renamed and saved (and closed) the sprites as you describe and of course it didn’t crash. I tried it a couple of times. If I could reproduce it on RPCEmu I might be able to help because I could enable my custom build that dumps all the recent branches and register values to a log when there’s an exception.

At the moment I’m tweaking a special build of Paint that automatically (using wimpt_fake_event()) makes random sprites into brushes and paints onto other random sprites (to try and reproduce the infamous Crash on Brush Selection, and no, so far it hasn’t reproduced it!), so maybe later I can tweak that to automate testing of renaming a large number of sprites. Of course there’s no guarantee that would reproduce your crashes either.

As for your issue with sprites being highlighted when you don’t want them to be, I’m afraid I consider that out of scope for the Paint Bounty, especially if ROOL decided to make that change.

I agree that it’s not part of this bounty. It does appear to be a bug though; the change log says it was to behave like Filer, which doesn’t highlight files when they’re dropped. Changing that would probably fix it, but again it’s not part of this bounty.

As I recall, the last element of a copy/move with Filer is to deselect the object/objects being acted on.
That’s clearly missing from the matching code in paint.

as you describe and of course it didn’t crash.

Of course. That’s the way it goes. I have exactly the same issue with the cache thingy in Manga. I just can’t get it to crash for me… :-(

I could enable my custom build that dumps all the recent branches and register values to a log when there’s an exception.

You probably ought to make that available, it sounds like a useful build for developers.

to try and reproduce the infamous Crash on Brush Selection, and no, so far it hasn’t reproduced it!

Brush? As in the circle, square, whatever thingy? Hmm… I’ve used that a few times with custom sprites to basically paste a sprite into an existing one, and it’s not crashed for me either. :-)

I’m afraid I consider that out of scope for the Paint Bounty,

Fair enough. I thought I’d mention it since you’re familiar with the code and it’s probably a one-line fix for the auto-selection part which, while not fixing the entire problem (there’s the nuke-selected issue), would at least get drag-loads working again.

especially if ROOL decided to make that change.

But clearly didn’t test it enough…

I’ll file a bug report.

which doesn’t highlight files when they’re dropped.

That, plus the fact that the Filer also doesn’t wipe out random whatever-is-selected when a file is dropped. It only overwrites a file of the same name and, additionally, if you have set the options to not confirm beforehand.

That’s clearly missing from the matching code in paint.

That would be the simplest fix, just don’t auto-select a dragged-in sprite.

Edit: Whoo-eee… This is broken beyond belief. I dragged my big sprite file into an empty sprite list window, and it loaded all of the sprites (about 50) and they were all marked as selected. I then dragged in a single sprite and it silently deleted them all leaving just the one dragged in last.

Any chance of adding your debugging code to RPCem sources – with a config option?

Could be very useful for debugging awkward code :-)

I’ll file a bug report.

Done -https://www.riscosopen.org/tracker/tickets/520

I’ve added a short video too, comparing behaviour in RISC OS 3.11 with current. ;-)

That would be the simplest fix, just don’t auto-select a dragged-in sprite.

Nulls out the handle for the selection in Filer as the copy into the destination completes, IIRC

Nulls out the handle for the selection in Filer as the copy into the destination completes, IIRC

Yes.

…Desktop.Filer.s.DragEnd around line 733:

 [ clearsel_file_copy
        MOV     r2, #Nowt
        BL      ClearAllSelections
        BL      NobbleMenuTree

(clearsel_file_copy is defined as TRUE in Options)

You probably ought to make that available, it sounds like a useful build for developers.

Any chance of adding your debugging code to RPCem sources – with a config option?

The code is horrible at the moment. Apart from being messy and horrendously slow, from what I remember it’s very inconsistent in what it does or doesn’t log (pretty much just the bits I thought I needed for certain bugs) and I think in some cases it misrepresents the exact form of the instructions it encounters; that’s because there are so many different variations that I’d effectively be writing my own disassembler to turn them all into the right text. And yes I could probably make use of an existing disassembler but that’s a lot more work I don’t have time for at the moment.

I may make the code available regardless at some point but it will be very much as-is.

I’m now looking at this zoom issue Jeffrey raised a while ago:

Minor feedback: When using Ctrl + scrollwheel to zoom out it’s possible for the window to shrink and no longer lie under the mouse, so you have to keep chasing it around the screen. It would probably be better if the window position + scroll offsets were adjusted to make sure the pixel that’s underneath the mouse stays in the same place on screen (both for zooming in & zooming out). I think that’s basically the same way that most other software handles scrollwheel-based zooming.

Adjusting the window position to keep the same pixel under the mouse isn’t as straightforward as it first appears, because parts of the window could start to disappear off the edge of the screen. In fact, when looking at this I found another subtle bug in Paint: drag a sprite window as far as you can off the left hand side of the screen and then zoom out a few times (with Ctrl-Q or the magnifier) and the window sometimes disappears completely off the edge of the screen!

I’ve been looking at how some other programs handle zooming and on Linux the norm seems to be to leave the window position, size and shape untouched and only vary the visible area of the document when zooming. Even Draw seems to do this too, at least until you zoom out so far that the entire document is visible – then it has to shrink the window. My copy of the Style Guide has nothing to say on the matter.

So, do we think it would be OK to change Paint so the magnifier tool and the keyboard and mouse wheel zoom controls don’t alter the window position and dimensions, only the visible area of the sprite and the scroll bar positions?

In the case of zooming out where the sprite window still has to shrink because the entire sprite is already visible, I suppose then we can safely move the window down and to the right so that the mouse pointer stays over the window.

What do you think?

I presume from the lack of any replies that no-one reading this has any objections to future builds of Paint keeping the window the same size when zooming with the magnifier, mouse wheel or keyboard shortcuts. This will be a change from the longstanding behaviour of always growing the window when zooming in and shrinking it when zooming out.

If a user wants to change the window size whilst zooming, they can hold down Ctrl and drag the resize icon.

I’m absolutely fine with your suggestion. Seems more intuitive than the whole sprite window changing size.

And great work you’ve done on Paint. I would hate to go back to using a pre-Andy Paint version.

Seems more intuitive than the whole sprite window changing size.

I, too, think keeping it the same size is correct. The user is changing the zoom of the content, not the size of window.
The addition to resize with Ctrl is an extra bonus for those who do want to change zoom and window size.

I would hate to go back to using a pre-Andy Paint version.

+1

Thanks guys, I appreciate the feedback. I think that change will result in slightly cleaner code as well. :)

Good news! I got Paint to crash the Filer (it’s always the Filer!) when doing a rename operation. And it only crashed the one time (not multiples), so the exception dump logfile shows what happened.

It looks like the act of renaming the sprite wasn’t the actual problem, it’s something to do with how the caret is being handled. Not sure if it’s Paint that’s doing it wrong, or the Filer. I suspect Paint because the caret is kind of important in a UI and… ;-)

Here’s the log:

Error block: 80000002 Internal error: abort on data transfer at &FC156990
R0 = 20613f18
R1 = 000168b0
R2 = 203e718c
R3 = 00000166
R4 = 00000278
R5 = 00000166
R6 = 00000278
R7 = 0000013a
R8 = 00000378
R9 = 00000166
R10 = 20613f18
R11 = 2001e5d4
R12 = 2001e174
R13 = fa207ee0
R14 = 2001e594
R15 = fc156998
CPSR = 60000113
R13_usr = 00009d70
R14_usr = 00000001
R13_svc = fa207ee0
R14_svc = 2001e594
SPSR_svc = 20000113
R13_irq = fa102000
R14_irq = 60000110
SPSR_irq = 60000110
R13_abt = fa301fa8
R14_abt = fc156998
SPSR_abt = 60000113
R13_und = fa402000
R14_und = fc264848
SPSR_und = 20000110
OSMem16: 2 = fa100000, 00002000, 00002000
OSMem16: 3 = fa200000, 00008000, 00008000
OSMem16: 4 = fa300000, 00002000, 00002000
OSMem16: 5 = fa400000, 00002000, 00002000
Memory: fa100000 - fa102000
Memory: fa200000 - fa208000
Memory: fa300000 - fa302000
Memory: fa400000 - fa402000
Memory: ffff0108 - ffff010c
OSRSI6: 69 = ffff0108

R15 = fc156998 = WindowManager +14df4 = rdrmlp1 +38
R14_svc = 2001e594 = Module area +1e594

SVC stack:
fa207ee0 : 203e718c :
fa207ee4 : 00000166 :
fa207ee8 : 00000278 :
fa207eec : 0000013a :
fa207ef0 : 00000166 :
fa207ef4 : 070f0018 : - R1
fa207ef8 : fc14b6a8 : | R14: fc14b6a8 (ASM call to fc156954)
         :          : | fc14b6a8 = WindowManager +9b04
         :          : | = int_get_rectangle +68
         :          : | fc156954 = WindowManager +14db0
         :          : | = redrawmenu +0
fa207efc : fc151908 : - R14: fc151908 (ASM call to fc14b640)
         :          : | fc151908 = WindowManager +fd64
         :          : | = uiclp +4
         :          : | fc14b640 = WindowManager +9a9c
         :          : | = int_get_rectangle +0
fa207f00 : 00000000 : - R2
fa207f04 : 00000000 : | R3
fa207f08 : 00000434 : | R4
fa207f0c : 00000800 : | R5
fa207f10 : 60000113 : | R6
fa207f14 : 000007dc : | R7
fa207f18 : 00000434 : | R8
fa207f1c : 00000800 : | R9
fa207f20 : fc1533a0 : | R14: fc1533a0 (ASM call to fc151858)
         :          : | fc1533a0 = WindowManager +117fc
         :          : | = exitsetcaret_abort +2c
         :          : | fc151858 = WindowManager +fcb4
         :          : | = int_set_icon_state +0
fa207f24 : 60000113 :
fa207f28 : 00003a1c :
fa207f2c : 20408959 : - R0
fa207f30 : ffffffff : | R1
fa207f34 : 00000000 : | R2
fa207f38 : 00000000 : | R3
fa207f3c : 00000000 : | R4
fa207f40 : ffffffff : | R5
fa207f44 : 203eceac : | R6
fa207f48 : 203ed3e8 : | R7
fa207f4c : 00009e00 : | R11
fa207f50 : fc156c38 : | R14: fc156c38 (ASM call to fc152e5c)
         :          : | fc156c38 = WindowManager +15094
         :          : | = unsetmenucaret +1c
         :          : | fc152e5c = WindowManager +112b8
         :          : | = int_set_caret_position +0
fa207f54 : 00000008 :                                            - R1
fa207f58 : 24251698 :                                            | R2
fa207f5c : 00000000 :                                            | R3
fa207f60 : 203d7758 :                                            | R4
fa207f64 : 000000ed :                                            | R5
fa207f68 : fc143a8c : - Return to fc143a8c?                      | R10
         :          : | = WindowManager +1ee8                    | 
         :          : | = Wimp_SWIdecode +0                      | 
fa207f6c : 00009e00 :                                            | R11
fa207f70 : fc156b60 :                                            | R14: fc156b60 (ASM call to fc156c1c)
         :          :                                            | fc156b60 = WindowManager +14fbc
         :          :                                            | = closmlp +48
         :          :                                            | fc156c1c = WindowManager +15078
         :          :                                            | = unsetmenucaret +0
fa207f74 : 20613f19 :
fa207f78 : 00000008 :
fa207f7c : 24251698 :
fa207f80 : 00000000 :
fa207f84 : 203d7758 :
fa207f88 : 000000ed :
fa207f8c : fffffffc :
fa207f90 : 00000008 :
fa207f94 : 20021934 :                                            - R1
fa207f98 : 24251698 :                                            | R2
fa207f9c : 00000000 :                                            | R3
fa207fa0 : 203d7758 :                                            | R4
fa207fa4 : 000000ed :                                            | R5
fa207fa8 : fc143a8c : - Return to fc143a8c?                      | R10
         :          : | = WindowManager +1ee8                    | 
         :          : | = Wimp_SWIdecode +0                      | 
fa207fac : fc14c408 :                                            | R14: fc14c408 (ASM call to fc156b10)
         :          :                                            | fc14c408 = WindowManager +a864
         :          :                                            | = repollwimp +0
         :          :                                            | fc156b10 = WindowManager +14f6c
         :          :                                            | = closemenus +0
fa207fb0 : 00009e00 :
fa207fb4 : 24251698 :
fa207fb8 : 00000000 :
fa207fbc : 00009dfc :
fa207fc0 : 00001800 :
fa207fc4 : 203eceac :
fa207fc8 : 203ed3e8 :
fa207fcc : 00000000 :
fa207fd0 : fc429e94 :
fa207fd4 : fc143a8c : - Return to fc143a8c?
         :          : | = WindowManager +1ee8
         :          : | = Wimp_SWIdecode +0
fa207fd8 : 00000007 :
fa207fdc : 00003a20 :
fa207fe0 : abb8aaa2 :
fa207fe4 : fc0106a8 : - fc017680 return to fc0106a8?
         :          : | fc017680 = +17680 in the Kernel
         :          : | = CallVector +0
         :          : | fc0106a8 = +106a8 in the Kernel
         :          : | = VectorUserSWI +8
fa207fe8 : 20000110 : - PSR?
fa207fec : 000600c7 : | SWI XWimp_Poll
fa207ff0 : fc181204 : | R14: fc181204
         :          : | = SharedCLibrary +19ea8
         :          : | = wimp_poll +3c
fa207ff4 : 00008230 : | R10
fa207ff8 : 00009db8 : | R11
fa207ffc : 203ebfb4 : | R12

R14_usr = 00000001 is system workspace

USR stack:
00009d70 : 00000000 :
[...snipped...all zeros...]


End of dump

And:

*Where
Address &FC156998 is at offset &00014DF4 in module 'WindowManager'
*ShowRegs
Register dump (stored at &20003110) is:
R0  = 20613F18 R1  = 000168B0 R2  = 203E718C R3  = 00000166
R4  = 00000278 R5  = 00000166 R6  = 00000278 R7  = 0000013A
R8  = 00000378 R9  = 00000166 R10 = 20613F18 R11 = 2001E5D4
R12 = 2001E174 R13 = FA207EE0 R14 = 2001E594 R15 = FC156998
Mode SVC32 flags set: nZCvqjggggeAift        PSR = 60000113
*MemoryI PC-20 +40
FC156978 : øÿÿ. : 1AFFFFF8 : BNE     &FC156960
FC15697C : †.-é : E92D029C : STMDB   R13!,{R2-R4,R7,R9}
FC156980 : Bî…â : E28CEE42 : ADD     R14,R12,#&0420     ; =1056
FC156984 : ..fiç : E79E1001 : LDR     R1,[R14,R1]
FC156988 : ...ã : E3110003 : TST     R1,#3
FC15698C : ]... : 1A00005D : BNE     &FC156B08
FC156990 : . .å : E5112008 : LDR     R2,[R1,#-8]
FC156994 : .0.å : E5113004 : LDR     R3,[R1,#-4]
FC156998 < ..Úå : E5DA0106 : LDRB    R0,[R10,#262]
FC15699C : ©èÿë : EBFFE8A9 : BL      &FC150C48
FC1569A0 : ð. ã : E3A000F0 : MOV     R0,#&F0            ; ="ð"
FC1569A4 : .ëÿë : EBFFEB1D : BL      &FC151620
FC1569A8 : ÑIà : E04990C3 : SUB     R9,R9,R3,ASR #1
FC1569AC : .‘Ià : E0499002 : SUB     R9,R9,R2
FC1569B0 : .@’å : E5914000 : LDR     R4,[R1,#0]
FC1569B4 : .p á : E1A07009 : MOV     R7,R9
*

Hope this helps.

That could be helpful, thanks Rick. I’ve just had a quick look at Paint’s code and when you’re using the Alt and click to rename, it uses wimp_create_icon() to make a new writable icon every time a rename is initiated and it does then do a wimp_set_caret_pos() at that point (and I can’t see anything obviously wrong with that code itself).

When you press Enter, I think Paint relies on the WIMP to give the caret back to its previous owner – in the case of the Alt-click-rename that will presumably happen when wimp_delete_icon() is called.

For the menu-driven rename, I think the placement of the caret on the rename writable icon is entirely handled by the WIMP (with the possible exception of any RISC_OSLib menu code).

It’s interesting your stack dump above mentions “unsetmenucaret” and then looks like it might be crashing trying to redraw an icon. It’s also interesting that you get it to crash both on the menu and the Alt-Click rename icon because there’s not much in common between them (maybe something’s corrupted memory as you suggested earlier).

When I do renames, if I have a colour picker open, the caret goes to that afterwards but if your sprites were 256-colour or less it’s likely nothing in Paint would be open to receive the caret so the WIMP might try to pass it to whatever program you were previously using it in.

Hang on though Rick, you said the Filer raised the exception. So doesn’t that imply that stack trace was doing stuff with Filer icons and menus?

When you press Enter, I think Paint relies on the WIMP to give the caret back to its previous owner – in the case of the Alt-click-rename that will presumably happen when wimp_delete_icon() is called.

For the menu-driven rename, I think the placement of the caret on the rename writable icon is entirely handled by the WIMP (with the possible exception of any RISC_OSLib menu code).

It’s not related to this bug, is it? Since the clipboard-in-icon work, the Wimp seems to have a bit of an unfortunate habit of trying to return the caret to places that it really shouldn’t… Given the lack of interest in that thread, I suspect the problem is still there and waiting to bite people.

It’s not related to this bug, is it? Since the clipboard-in-icon work, the Wimp seems to have a bit of an unfortunate habit of trying to return the caret to places that it really shouldn’t… Given the lack of interest in that thread, I suspect the problem is still there and waiting to bite people.

It certainly sounds very similar, doesn’t it? I’ve just replied to your thread with a link to yet another relevant thread, Steve.

Rick, this is where it died in your last crash dump:

;------------------------------------------------------------------------------

; Draw dotted lines in body of a menu
; Entry:  x0,y1 = origin of window (assume scx = 0)
;         x1 = rhs of window on screen (for drawing dotted line across)
;         handle --> window definition

redrawmenu
        Push    "R1,LR"

        ADR     R14,menuhandles
        LDR     R1,menuSP
rdrmlp1
        CMP     R1,#0
        Pull    "R1,PC",LT
        LDR     R0,[R14,R1]
        Abs     R0,R0
        CMP     R0,handle               ; is it this one?
        SUBNE   R1,R1,#4                ; try again
        BNE     rdrmlp1

; get menu data and draw appropriate dotted lines

        Push    "R2-R4,y0,y1"

        ADR     R14,menudata
        LDR     R1,[R14,R1]             ; get pointer to data (omits header)
        TST     R1,#3
        BNE     exitdottedlines         ; it's probably a dbox

        LDR     R2,[R1,#m_height-m_header]
 [ MinimumMenuHeight
	CMP	R2,#44
	MOVLO	R2,#44
 ]
        LDR     R3,[R1,#m_gap-m_header]

        LDRB    R0,[handle,#w_wfcol]    ; **** This was Rick's crash ****
        BL      foreground
        MOV     R0,#&F0
        BL      setdotdash

        SUB     y1,y1,R3,ASR #1         ; subtract 1/2 gap from top
rdrmlp2
        SUB     y1,y1,R2                ; subtract icon height
        LDR     R4,[R1,#mi_mflags]
        MOV     y0,y1                   ; remember y1

So it looks like it died trying to read the work area foreground colour from a menu’s window handle. Maybe it was a menu that had gone away. Surely not an earlier incarnation of the Rename submenu that the caret got passed back to?

That crash was from renaming via the menu. The rename happens okay. If I remember correctly, the menu remains open with the rename option hanging off of it, and empty.

Really have no idea why the Filer is getting involved in any of this. Is the Wimp getting fooled into doing some sort of nasty task swap shenanigans?

Really have no idea why the Filer is getting involved in any of this. Is the Wimp getting fooled into doing some sort of nasty task swap shenanigans?

Rick can you remember if you had opened any menus of the Filer or perhaps Alt-clicked a file in a Filer window so it received the caret before Paint?

The fact you say the Filer raised the error just gives me the feeling the WIMP had tried to hand the caret back to a Filer menu. I wish we knew what owned that window handle at &20613F18.

Edit: It’s not at all clear to me on that stack trace why we went from a wimp_poll to int_set_caret_position. Is this some internal caret-handling code that runs (potentially) every time an application calls wimp_poll()?

That exitsetcaret_abort is interesting too. It’s in CnPCaret but at the moment I don’t understand under what circumstances it’s supposed to be called. Why the “abort”?

Sorry, I don’t remember. I don’t think I would have made copying/renaming that much, as it’s easier just to save directly from Paint with a new filename.

The Filer quite possibly had input focus as a result of double clicking a sprite file, but no caret is associated with that.

Why the “abort”?

Oh, that one I think I can answer. ;-)
It’s trying to load something from a menu handle pointing to &168B0. This is in application space. It might make sense for Paint (it’s about 60K from the start of app workspace, does this seem right?), but since it is Filer throwing the error, that’s a module task, which doesn’t have a Wimp slot. So what’s at that address when the Filer is active? Fweeee-BOOM!

You might be on to something.

I renamed the sprite file in the Filer, loaded it, and renamed it in Paint.
Boom. [but doing it again didn’t repeat the error…?]

I didn’t have the exception dump running (didn’t think), but…

*Where
Address &FC156998 is at offset &00014DF4 in module 'WindowManager'
*ShowRegs
Register dump (stored at &20003110) is:
R0  = 20440958 R1  = 000168B0 R2  = 203F918C R3  = 00000166
R4  = 0000022E R5  = 00000166 R6  = 0000022E R7  = 0000013A
R8  = 0000032E R9  = 00000166 R10 = 20440958 R11 = 2001E5D4
R12 = 2001E174 R13 = FA207EE0 R14 = 2001E594 R15 = FC156998
Mode SVC32 flags set: nZCvqjggggeAift        PSR = 60000113
*

So it looks to me to be the same as before.

Looking in Paint’s workspace at that location:

00016870 : 00000000 00000000 00000000 00000400 : ................
00016880 : 00000000 00000000 00000000 3E694C3C : ............<Li>
00016890 : 00000038 616E6552 0000656D 00000000 : 8...Rename......
000168A0 : 00070207 000000D0 0000002C 00000000 : ....Ð...,.......
000168B0 : 00000084 FFFFFFFF 0700F139 2038E90C : ✘...ÿÿÿÿ9ñ...é8
000168C0 : FC435720 0000000D 00000000 3E694C3C :  WCü........<Li>

Now, this looks to me (just a cursory glance) to be the menu block for the stand-alone “Rename” menu object.

The really big question here is… how/why the hell is the Filer trying to access Paint’s menu?