View Issue Details

IDProjectCategoryView StatusLast Update
0014876LazarusIDEpublic2020-04-13 13:50
ReporterGraeme Geldenhuys Assigned ToLudo Brands  
PrioritynormalSeverityfeatureReproducibilityalways
Status resolvedResolutionfixed 
Platformx86OSUbuntu Linux 
Product Version0.9.29 (SVN) 
Summary0014876: No conflict warning when External Tools uses keyboard shortcut
DescriptionEven with v0.9.29, if you setup a keyboard
shortcut in the External Tools, it never tells you if it conflicts
with another keyboard shortcut. To find that out, you have to go to
"Environment > Options > Key Mappings" and then click the "Check
Consistency" button.
TagsNo tags attached.
Fixed in Revisionr42790
LazTarget-
Widgetset
Attached Files

Relationships

related to 0024008 resolvedJuha Manninen problem with CTRL key in two-key shortcut 

Activities

Graeme Geldenhuys

2009-10-23 08:47

reporter   ~0031608

How can you mark this as a feature request? The External Tools dialog is part of the IDE. Setting up a keyboard shortcut is also part of the IDE features. So all these should work the same as any other keyboard shortcut setup, because ultimately the keyboard shortcuts (no matter from where they are defined), are registered in same place (internally) in the IDE. But for some reason the conflict test is not performed automatically from the External Tools dialog.

The keyboard shortcut setup is not consistent throughout the IDE. So this is definitely a bug and *not* a feature request.

Vincent Snijders

2009-10-23 09:14

manager   ~0031609

So, the feature is:
* Perform conflict test from the External Tools dialog.

Setting up a keyboard shortcut is part of the IDE features. It is offered in several places. Somebody forget to implement this feature in the External Tools Dialog. So, it is an unimplemented feature.

If you want to discuss this further, please use the mailing list.

Vincent Snijders

2009-11-17 20:15

manager   ~0032219

I compared the two screens and I did not see a good place to perform this check in a common place, because the configure external tools dialog doesn't have a modal dialog to enter short cuts.

Ludo Brands

2013-05-04 12:38

developer  

14876.diff (6,060 bytes)   
Index: ide/exttooldialog.pas
===================================================================
--- ide/exttooldialog.pas	(revision 41020)
+++ ide/exttooldialog.pas	(working copy)
@@ -65,6 +65,7 @@
     fOnFreeOutputFilter: TOnFreeOutputFilter;
     fOnNeedsOutputFilter: TOnNeedsOutputFilter;
     fRunningTools: TList; // list of TProcess
+    fAllKeys:TKeyCommandRelationList;
     function GetToolOpts(Index: integer): TExternalToolOptions;
     procedure SetToolOpts(Index: integer; NewTool: TExternalToolOptions);
     procedure AddRunningTool(TheProcess: TProcess; ExecuteProcess: boolean);
@@ -224,6 +225,7 @@
       TProcess(fRunningTools[i]).Free;
     fRunningTools.Free;
   end;
+  fAllKeys.Free;
   inherited Destroy;
 end;
 
@@ -277,6 +279,9 @@
   i: integer;
   KeyCommandRelation: TKeyCommandRelation;
 begin
+  if not assigned(fAllKeys) then
+    fAllKeys:=TKeyCommandRelationList.Create;
+  fAllKeys.Assign(KeyCommandRelationList);
   for i:=0 to Count-1 do begin
     KeyCommandRelation:=KeyCommandRelationList.FindByCommand(ecExtToolFirst+i);
     if KeyCommandRelation<>nil then begin
@@ -608,7 +613,7 @@
     exit;
   end;
   NewTool:=TExternalToolOptions.Create;
-  if ShowExtToolOptionDlg(fTransferMacros,NewTool)=mrOk then begin
+  if ShowExtToolOptionDlg(fTransferMacros,NewTool,TExternalToolList(EnvironmentOptions.ExternalTools).fAllKeys)=mrOk then begin
     fExtToolList.Add(NewTool);
     Listbox.Items.Add(ToolDescription(fExtToolList.Count-1));
   end else begin
@@ -685,7 +690,7 @@
 begin
   i:=Listbox.ItemIndex;
   if i<0 then exit;
-  if ShowExtToolOptionDlg(fTransferMacros,fExtToolList[i])=mrOk
+  if ShowExtToolOptionDlg(fTransferMacros,fExtToolList[i],TExternalToolList(EnvironmentOptions.ExternalTools).fAllKeys)=mrOk
   then begin
     Listbox.Items[i]:=ToolDescription(i);
     EnableButtons;
Index: ide/exttooleditdlg.pas
===================================================================
--- ide/exttooleditdlg.pas	(revision 41020)
+++ ide/exttooleditdlg.pas	(working copy)
@@ -47,7 +47,7 @@
   Dialogs, ExtCtrls, LCLProc, ButtonPanel,
   IDEMsgIntf, IDEExternToolIntf, IDEHelpIntf,
   PropEdits, IDEDialogs, TransferMacros, LazarusIDEStrConsts,
-  EditMsgScannersDlg;
+  EditMsgScannersDlg, KeyMapping;
 
 type
   { TExternalToolOptions }
@@ -98,11 +98,13 @@
     procedure OpenButtonClick({%H-}sender : TOBject);
     procedure ScannersButtonClick(Sender: TObject);
   private
+    fAllKeys:TKeyCommandRelationList;
     fOptions: TExternalToolOptions;
     fTransferMacros: TTransferMacroList;
     fScanners: TStrings;
     fKeyBox: TShortCutGrabBox;
     procedure FillMacroList;
+    function KeyConflicts(Key:word;Shift:TShiftState):TModalResult;
     procedure LoadFromOptions;
     procedure SaveToOptions;
     procedure UpdateButtons;
@@ -118,19 +120,22 @@
 
 
 function ShowExtToolOptionDlg(TransferMacroList: TTransferMacroList;
-  ExternalToolOptions: TExternalToolOptions):TModalResult;
+  ExternalToolOptions: TExternalToolOptions;
+  AllKeys:TKeyCommandRelationList):TModalResult;
   
 implementation
 
 {$R *.lfm}
 
 function ShowExtToolOptionDlg(TransferMacroList: TTransferMacroList;
-  ExternalToolOptions: TExternalToolOptions):TModalResult;
+  ExternalToolOptions: TExternalToolOptions;
+  AllKeys:TKeyCommandRelationList):TModalResult;
 var ExternalToolOptionDlg: TExternalToolOptionDlg;
 begin
   Result:=mrCancel;
   ExternalToolOptionDlg:=TExternalToolOptionDlg.Create(nil);
   try
+    ExternalToolOptionDlg.fAllKeys:=AllKeys;
     ExternalToolOptionDlg.Options:=ExternalToolOptions;
     ExternalToolOptionDlg.MacroList:=TransferMacroList;
     Result:=ExternalToolOptionDlg.ShowModal;
@@ -322,6 +327,60 @@
   MacrosListbox.Items.EndUpdate;
 end;
 
+function TExternalToolOptionDlg.KeyConflicts(Key: word; Shift: TShiftState
+  ): TModalResult;
+type
+  TConflictType = (ctNone,ctConflictKeyA,ctConflictKeyB);
+var
+  i: Integer;
+  ct:TConflictType;
+  CurName: TCaption;
+  ConflictName: String;
+begin
+  Result:=mrOK;
+  // look if we have already this key
+  if Key=VK_UNKNOWN then
+    exit;
+  i:=0;
+  for i:=0 to fAllKeys.RelationCount-1 do
+    begin
+    with fAllKeys.Relations[i] do
+      begin
+      ct:=ctnone;
+      if (ShortcutA.Key1=Key) and (ShortcutA.Shift1=Shift) then
+        ct:=ctConflictKeyA
+      else if (ShortcutB.Key1=Key) and (ShortcutB.Shift1=Shift) then
+        ct:=ctConflictKeyB;
+      if (ct<>ctNone) then begin
+        CurName:=TitleEdit.Text;
+        ConflictName:=GetCategoryAndName;
+        if ct=ctConflictKeyA then
+          ConflictName:=ConflictName
+                    +' ('+KeyAndShiftStateToEditorKeyString(ShortcutA)
+        else
+          ConflictName:=ConflictName
+                   +' ('+KeyAndShiftStateToEditorKeyString(ShortcutB);
+        case IDEMessageDialog(lisPEConflictFound,
+           Format(lisTheKeyIsAlreadyAssignedToRemoveTheOldAssignmentAnd, [
+             KeyAndShiftStateToKeyString(Key,Shift), LineEnding, ConflictName, LineEnding,
+             LineEnding, LineEnding, CurName]), mtConfirmation, [mbYes, mbNo, mbCancel])
+        of
+          mrYes:    Result:=mrOK;
+          mrCancel: Result:=mrCancel;
+          else      Result:=mrRetry;
+        end;
+        if Result=mrOK then begin
+          if (ct=ctConflictKeyA) then
+            ShortcutA:=ShortcutB;
+          ClearShortcutB;
+        end
+        else
+          break;
+      end;
+      end;
+    end;
+end;
+
 procedure TExternalToolOptionDlg.SetComboBox(
   AComboBox: TComboBox; const AValue: string);
 var i: integer;
@@ -364,6 +423,17 @@
 
 procedure TExternalToolOptionDlg.OKButtonClick(Sender: TObject);
 begin
+  case KeyConflicts(fKeyBox.Key,fKeyBox.ShiftState) of
+    mrCancel:   begin
+        debugln('TExternalToolOptionDlg.OkButtonClick KeyConflicts failed for key1');
+        ModalResult:=mrCancel;
+        exit;
+      end;
+    mrRetry: begin
+        ModalResult:=mrNone;
+        exit;
+      end;
+    end;
   if (TitleEdit.Text<>'') and (FilenameEdit.Text<>'') then
     SaveToOptions
   else begin
14876.diff (6,060 bytes)   

Ludo Brands

2013-05-04 12:38

developer   ~0067413

attached a patch against revision 41020.

This solves the above bug/feature request. However, during testing it appears that the "Check Consistency" in Options->Editor->Key Mapping does not verify against the keys already assigned in external tools.

Juha Manninen

2013-05-04 13:29

developer   ~0067414

Ok, I didn't look carefully how it is done now. The main Key Mapping window should absolutely check all shortcuts.

Now the design is flawed. You re-implemented the conflicting key check function. The original function may change for some reason, for example due to related issue 0024008, and then we must remember to update the copied code. Not good.
The main key mapping should have a hook for shortcuts defined elsewhere, or maybe all shortcuts should be saved there in the first place. I don't know what is the best way to implement it.

I suggest to implement it differently anyway. Let's not hurry with it. Duplicate code and partly broken behavior will only cause headache later.
I know, I asked your help with this and now I am complaining...
That's how it goes. :)

Ludo Brands

2013-05-04 15:03

developer   ~0067418

Last edited: 2013-05-04 15:07

View 2 revisions

Well this a matter of the glass half empty or half full. External tools don't support 2 key short cuts and no alternate key sequences, just one single key combination. So 0024008 is not related to external tools. The scope for external tools is system wide, so scope checking as in TShortCutDialog.ResolveConflicts is not relevant. TShortCutDialog works also on another sets of keys. So the only part that is duplicated is the re-use of IDEMessageDialog(lisPEConflictFound,...) and afaik that function is meant to be re-used.

The problem with "Check Consistency" is an already existing problem, not related to the patch.

Juha Manninen

2013-09-14 09:11

developer   ~0070046

I applied the patch. Thanks Ludo.
It can be fine-tuned later.

Issue History

Date Modified Username Field Change
2009-10-22 16:48 Graeme Geldenhuys New Issue
2009-10-22 22:07 Vincent Snijders LazTarget => -
2009-10-22 22:07 Vincent Snijders Severity minor => feature
2009-10-23 08:47 Graeme Geldenhuys Note Added: 0031608
2009-10-23 09:14 Vincent Snijders Note Added: 0031609
2009-11-17 20:15 Vincent Snijders Note Added: 0032219
2009-11-17 20:16 Vincent Snijders Status new => acknowledged
2013-05-04 10:37 Ludo Brands Assigned To => Ludo Brands
2013-05-04 10:37 Ludo Brands Status acknowledged => assigned
2013-05-04 12:38 Ludo Brands File Added: 14876.diff
2013-05-04 12:38 Ludo Brands Note Added: 0067413
2013-05-04 13:15 Juha Manninen Relationship added related to 0024008
2013-05-04 13:29 Juha Manninen Note Added: 0067414
2013-05-04 15:03 Ludo Brands Note Added: 0067418
2013-05-04 15:07 Ludo Brands Note Edited: 0067418 View Revisions
2013-09-14 09:11 Juha Manninen Fixed in Revision => r42790
2013-09-14 09:11 Juha Manninen Note Added: 0070046
2013-09-14 09:11 Juha Manninen Status assigned => resolved
2013-09-14 09:11 Juha Manninen Resolution open => fixed