chrisrolliston / CCR.Clipboard

Extended TClipboard implementation for Delphi (FMX and VCL)
60 stars 15 forks source link

Missing UnregisterForDragAndDrop #1

Closed tueddy closed 9 years ago

tueddy commented 9 years ago

This method is missing (CCR.Clipboard.FMX.pas):

class procedure TClipboardHelper.UnregisterForDragAndDrop(const Form: TCommonCustomForm); begin if Assigned(FRegisteredDropTargets) and FRegisteredDropTargets.ContainsKey(Form) then FRegisteredDropTargets.Remove(Form); end;

If a form is registered and the form is beeing closed/freed, a dangling pointer remains in dictionary FRegisteredDropTargets. So please add this unregister method.

FRegisteredDropTargets is declared as TDictionary<Pointer, IClipboardDropInfo>;, why not declare as

TDictionary<TCommonCustomForm, IClipboardDropInfo>;?

Best regards Dirk

chrisrolliston commented 9 years ago

Hi Dirk

On the second point, your text has got mangled ('TDictionary;;' - 'TDictionary;?') - can you clarify please?

tueddy commented 9 years ago

Hi Chris,

the dictionary is defined as:

FRegisteredDropTargets: TDictionary<Pointer, IClipboardDropInfo>; And later FRegisteredDropTargets.Add(Pointer(Form), Result);

It should be

FRegisteredDropTargets: TDictionary<TCommonCustomForm, IClipboardDropInfo>; And later FRegisteredDropTargets.Add(Form, Result);

But the real problem was leaving an already freed form in dictionary.. Best regards Dirk

chrisrolliston commented 9 years ago

Hi Dirk

No, it's Pointer to make it a weak reference on mobile (the VCL version can declare as TCustomForm because ARC will never be an issue in a VCL context). I'll look into the main issue, though that's really an oversight - the code should internally register for free notifications and unregister a form automatically like the VCL version does. I'd rather not provide an explicit TClipboardHelper.UnregisterForDragAndDrop because it wouldn't be safe (the drag and drop support has to muck around in FMX internals).

tueddy commented 9 years ago

Aha, thanks for explaination! So it's for ARC scrumbling. Sad that strong typecast not valid here.. Best regards Dirk

chrisrolliston commented 9 years ago

ARC is a bit of a nightmare when dealing with component internals, standard collection classes and anonymous methods as you get implicit strong references (and so the potential for retain cycles) everywhere.

Anyhow, I've just committed small changes to implement the automatic unregistering. The dangling pointers wouldn't really have been an issue anyhow given they are never dereferenced anywhere, however they now shouldn't arise in the first place. Can you confirm the changes work as expected please?

tueddy commented 9 years ago

Hi Chris,

seems to work fine, thanks!!

chrisrolliston commented 9 years ago

Great, thanks for confirming