Unable to delete Resolution (and related items)

[Keith Jones]

Hi,

 This applies to all versions AFAIK. It's a bit tricky to tell as the 1.1.x code is an upgrade to the 1.0.6 version I was playing with (and heavily refactored) :-)

 Whilst naively doing the usual "dive into the software without reading the manual' I added a 1024x768 resolution without understanding that it was the aspect ratio that was important. When I tried to delete the custom resolution I found that nothing happened when I clicked the delete button.

 A quick trace of log files for 1.0.6 and... [sorry about the lack of patch files but this isn't my dev machine :-( ]

 1) Function DeleteForm in .\lib\pages\resolution.class.php lacks the calls to the form display so no result is returned and the delete always fails.

[ I'm doing this from memory of what I changed on my dev machine so please pardon the lack of accuracy and guesses at size values! ]

 At line 234 you need to insert something like;

         $response->SetFormRequestResponse($form, 'Confirm Delete', '250px', '150px');
         $response->Respond();

 ...to bring the delete confirmation up and get a valid return value.

 2) In the same file, line 228, $resolutionID is in lowercase, and as PHP is case sensitive, the wrong resolution table entry is likely to be deleted. It triggers an undefined variable at least ;-)

 I hope that makes sense!

 Might I supply a couple of 20:20 hindsight suggestions? From rather disasterous experiences myself, it's never a good idea to use variable names that are different only in case especially when they're also likely to be used as placeholders in strings or fields in databases. It might be worth using a prefix for local variables in the long run. [Sheesh, one bug report and I'm spouting about coding standards ;-) ]

 There are so many times I've destroyed my own code with just a simple search and replace and I'd rather not see all this good stuff drive you quite as mad as my code has !!! :-)

 Have fun,

Keith

PS: A last little wibble :-) The delete form looked a bit naff on my dev machine because all the text was left justified. Maybe a "xibofor.centered" css sub-class would be useful?

[Dan Garner (dangarner)]

Keith,

Thanks for your time on this! I've patched your fixes into my current server dev branch.

You are quite right to point out my slightly haphazard approach to variable naming :-) The resolution stuff was a bit of a "rush job" which didn't help, hence it clearly not being tested either!

The resolution form still needs the 1.1 spruce up too - which should make it more visually appealing... I'll look into making the text centre aligned when I do that. Unless you fancy doing it of course :-)

Thanks again!