kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

Ok, I have pushed the fixes for the rename tree update, and an attempt to
fix the item selection after rename. Since Gtk uses the rename event, I
can't test the add code path easily (it is probably time I actually tried
to set up my mac and Windows build environments). It would be good to
verify that code path on Windows and mac now (the GTK path works).

Mikołaj, thanks for pointing this out, helping to track down the problem,
and testing. It really helped!

-Ian

On Sat, 16 Nov 2019, 21:01 Mikołaj Wielgus, <wielgusmikolaj@xxxxxxxxx>
wrote:

> I've tested Ian's patch on Windows 10 and it works correctly for me, with
> exception to the same selection problem, as well.
>
> I've done simple logging and found out that the rename event routine is
> not executed during rename, but the delete and create event routines are,
> in this order.
>
> Interestingly, the wxWidgets v3.0.4 reference manual states:
> "Implementation limitations: this class is currently implemented for MSW,
> OS X and GTK ports but doesn't detect all changes correctly everywhere:
> under MSW accessing the file is not detected (only modifying it is) and
> under OS X neither accessing nor modifying is detected (only creating and
> deleting files is). Moreover, OS X version doesn't currently collapse pairs
> of create/delete events in a rename event, unlike the other ones.".
>
> The phrase "unlike the other ones" means that the Windows port should
> collapse create/delete events into a rename event, which is not the case on
> my build.
>
> Best regards,
> Mikołaj Wielgus
>
>
> On Sat, Nov 16, 2019 at 5:21 PM Ian McInerney <Ian.S.McInerney@xxxxxxxx>
> wrote:
>
>> Thanks for testing. I had noticed the selection issue on GTK as well, so
>> I will fix it up before I push it.
>>
>> The file system watcher is a finiky creature. GTK for sure uses the
>> rename events (I traced those through the logic last night), but according
>> to the docs (
>> https://docs.wxwidgets.org/3.1/classwx_file_system_watcher.html) OSX
>> can't create a rename event, so we get a delete and create event
>> separately. The selection issue is easy to fix in the rename event, since I
>> just need to select the new item there. It is slightly more difficult for
>> OSX though, since we don't always want to have a newly create file be
>> selected. I can probably work around it though.
>>
>> I will let you know when I push the change so you can test it.
>>
>> -Ian
>>
>> On Sat, 16 Nov 2019, 2:24 p.m. Jeff Young, <jeff@xxxxxxxxx> wrote:
>>
>>> How bizarre.  Make you wonder how it worked on Mac and Windows.  (Hmmm…
>>> I wonder if GTK goes through wxFSW_EVENT_RENAME and Mac and Windows go
>>> through wxFSW_EVENT_DELETE and wxFSW_EVENT_CREATE?)
>>>
>>> Your patch works on OSX except for one thing: it doesn’t get the
>>> selection right (selecting the next item in the moved-from location rather
>>> than keeping the moved item selected).
>>>
>>> Cheers,
>>> Jeff.
>>>
>>>
>>> On 16 Nov 2019, at 01:57, Ian McInerney <Ian.S.McInerney@xxxxxxxx>
>>> wrote:
>>>
>>> Ok, so I dug into this some more and first of all, the changes that Jeff
>>> pushed still didn't work on GTK.
>>>
>>> The root cause of this doesn't appear to be our file system watcher, but
>>> was instead the fact that the tree was being improperly updated. First of
>>> all, we were renaming the old node with the new filename which was causing
>>> the deletion routine in the rename fswatcher handler to never find it and
>>> remove it. Second of all, we were adding the new file into the same
>>> directory as the original file, so it was looking like it was never leaving.
>>>
>>> I believe these two patches address the issues, and they work on my GTK
>>> box for both a rename moving directories and a rename inside the same
>>> directory. The first set just reverts the changes that Jeff made to the
>>> code to get it back to the state it was originally, and the second is the
>>> actual new changeset. @Jeff and @Mikolaj, if you could both test these
>>> changes on Windows and OSX to see how they behave that would be good. If
>>> they work for both of you I will push them.
>>>
>>> -Ian
>>>
>>> On Sat, Nov 16, 2019 at 1:00 AM Mikołaj Wielgus <
>>> wielgusmikolaj@xxxxxxxxx> wrote:
>>>
>>>> Jeff,
>>>>
>>>> your new version appears to be working well on my computer on Windows.
>>>>
>>>> Best regards,
>>>> Mikołaj Wielgus
>>>>
>>>>
>>>> On Sat, Nov 16, 2019 at 1:02 AM Jeff Young <jeff@xxxxxxxxx> wrote:
>>>>
>>>>> I pushed a smarter version of my original fix.  @Mikolaj & @Ian if you
>>>>> could test it on Windows and GTK that would be great.
>>>>>
>>>>> Cheers,
>>>>> Jeff.
>>>>>
>>>>>
>>>>> On 15 Nov 2019, at 22:27, Ian McInerney <Ian.S.McInerney@xxxxxxxx>
>>>>> wrote:
>>>>>
>>>>> Scratch my last. It is GTK with the problems. When I rename the file
>>>>> to a new directory with this patch, the tree never seems to update. I have
>>>>> to manually refresh it in order for the file to appear in the correct place.
>>>>>
>>>>> -Ian
>>>>>
>>>>> On Fri, Nov 15, 2019 at 10:21 PM Mikołaj Wielgus <
>>>>> wielgusmikolaj@xxxxxxxxx> wrote:
>>>>>
>>>>>> Yes, I'm on Windows (the details are in the linked related bug
>>>>>> report).
>>>>>>
>>>>>> Sorry for the return value problem -- I failed to notice the warnings
>>>>>> in console.
>>>>>>
>>>>>> Best regards,
>>>>>> Mikołaj Wielgus
>>>>>>
>>>>>>
>>>>>> On Fri, Nov 15, 2019 at 11:07 PM Ian McInerney <
>>>>>> Ian.S.McInerney@xxxxxxxx> wrote:
>>>>>>
>>>>>>> I'll give it a test on GTK once my build here finishes, but I don't
>>>>>>> think I have seen any issues with file watcher on GTK in the past.
>>>>>>>
>>>>>>> -Ian
>>>>>>>
>>>>>>> On Fri, Nov 15, 2019 at 10:03 PM Jeff Young <jeff@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>> Hi Mikolaj,
>>>>>>>>
>>>>>>>> The Mac compiler doesn’t like Rename() returning values when the
>>>>>>>> return type is void.  However, after fixing that it works fine on Mac.
>>>>>>>>
>>>>>>>> I remember however something about the file watcher not working on
>>>>>>>> all platforms.  I thought the problem platform was Windows, though, so
>>>>>>>> maybe I’m not remembering it correctly (as you’re on Windows, right?).
>>>>>>>>
>>>>>>>> Can someone validate that this works on GTK?
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Jeff.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 15 Nov 2019, at 21:05, Mikołaj Wielgus <wielgusmikolaj@xxxxxxxxx>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Renaming file to a different directory causes the entire project
>>>>>>>> tree to be recreated, which causes all subdirectories to collapse,
>>>>>>>> unexpectedly to the user.
>>>>>>>>
>>>>>>>> This patch solves the problem by deleting the original node after
>>>>>>>> moving the file. Then the file watcher raises an event whose handler
>>>>>>>> constructs a new node in the new location.
>>>>>>>>
>>>>>>>> This issue comes from the fix to this bug:
>>>>>>>> https://bugs.launchpad.net/kicad/+bug/1852431
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Mikołaj Wielgus
>>>>>>>> <0001-Do-not-reconstruct-proj-tree-on-rename-to-diff-dir.patch>
>>>>>>>> _______________________________________________
>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>
>>>>>>>
>>>>> <0002-Fix-renaming-of-files-in-the-project-tree.patch>
>>> <0001-Revert-changes-in-the-project-manager-file-tree-for-.patch>
>>>
>>>
>>>
>> On Sat, 16 Nov 2019, 2:24 p.m. Jeff Young, <jeff@xxxxxxxxx> wrote:
>>
>>> How bizarre.  Make you wonder how it worked on Mac and Windows.  (Hmmm…
>>> I wonder if GTK goes through wxFSW_EVENT_RENAME and Mac and Windows go
>>> through wxFSW_EVENT_DELETE and wxFSW_EVENT_CREATE?)
>>>
>>> Your patch works on OSX except for one thing: it doesn’t get the
>>> selection right (selecting the next item in the moved-from location rather
>>> than keeping the moved item selected).
>>>
>>> Cheers,
>>> Jeff.
>>>
>>>
>>> On 16 Nov 2019, at 01:57, Ian McInerney <Ian.S.McInerney@xxxxxxxx>
>>> wrote:
>>>
>>> Ok, so I dug into this some more and first of all, the changes that Jeff
>>> pushed still didn't work on GTK.
>>>
>>> The root cause of this doesn't appear to be our file system watcher, but
>>> was instead the fact that the tree was being improperly updated. First of
>>> all, we were renaming the old node with the new filename which was causing
>>> the deletion routine in the rename fswatcher handler to never find it and
>>> remove it. Second of all, we were adding the new file into the same
>>> directory as the original file, so it was looking like it was never leaving.
>>>
>>> I believe these two patches address the issues, and they work on my GTK
>>> box for both a rename moving directories and a rename inside the same
>>> directory. The first set just reverts the changes that Jeff made to the
>>> code to get it back to the state it was originally, and the second is the
>>> actual new changeset. @Jeff and @Mikolaj, if you could both test these
>>> changes on Windows and OSX to see how they behave that would be good. If
>>> they work for both of you I will push them.
>>>
>>> -Ian
>>>
>>> On Sat, Nov 16, 2019 at 1:00 AM Mikołaj Wielgus <
>>> wielgusmikolaj@xxxxxxxxx> wrote:
>>>
>>>> Jeff,
>>>>
>>>> your new version appears to be working well on my computer on Windows.
>>>>
>>>> Best regards,
>>>> Mikołaj Wielgus
>>>>
>>>>
>>>> On Sat, Nov 16, 2019 at 1:02 AM Jeff Young <jeff@xxxxxxxxx> wrote:
>>>>
>>>>> I pushed a smarter version of my original fix.  @Mikolaj & @Ian if you
>>>>> could test it on Windows and GTK that would be great.
>>>>>
>>>>> Cheers,
>>>>> Jeff.
>>>>>
>>>>>
>>>>> On 15 Nov 2019, at 22:27, Ian McInerney <Ian.S.McInerney@xxxxxxxx>
>>>>> wrote:
>>>>>
>>>>> Scratch my last. It is GTK with the problems. When I rename the file
>>>>> to a new directory with this patch, the tree never seems to update. I have
>>>>> to manually refresh it in order for the file to appear in the correct place.
>>>>>
>>>>> -Ian
>>>>>
>>>>> On Fri, Nov 15, 2019 at 10:21 PM Mikołaj Wielgus <
>>>>> wielgusmikolaj@xxxxxxxxx> wrote:
>>>>>
>>>>>> Yes, I'm on Windows (the details are in the linked related bug
>>>>>> report).
>>>>>>
>>>>>> Sorry for the return value problem -- I failed to notice the warnings
>>>>>> in console.
>>>>>>
>>>>>> Best regards,
>>>>>> Mikołaj Wielgus
>>>>>>
>>>>>>
>>>>>> On Fri, Nov 15, 2019 at 11:07 PM Ian McInerney <
>>>>>> Ian.S.McInerney@xxxxxxxx> wrote:
>>>>>>
>>>>>>> I'll give it a test on GTK once my build here finishes, but I don't
>>>>>>> think I have seen any issues with file watcher on GTK in the past.
>>>>>>>
>>>>>>> -Ian
>>>>>>>
>>>>>>> On Fri, Nov 15, 2019 at 10:03 PM Jeff Young <jeff@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>> Hi Mikolaj,
>>>>>>>>
>>>>>>>> The Mac compiler doesn’t like Rename() returning values when the
>>>>>>>> return type is void.  However, after fixing that it works fine on Mac.
>>>>>>>>
>>>>>>>> I remember however something about the file watcher not working on
>>>>>>>> all platforms.  I thought the problem platform was Windows, though, so
>>>>>>>> maybe I’m not remembering it correctly (as you’re on Windows, right?).
>>>>>>>>
>>>>>>>> Can someone validate that this works on GTK?
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Jeff.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 15 Nov 2019, at 21:05, Mikołaj Wielgus <wielgusmikolaj@xxxxxxxxx>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Renaming file to a different directory causes the entire project
>>>>>>>> tree to be recreated, which causes all subdirectories to collapse,
>>>>>>>> unexpectedly to the user.
>>>>>>>>
>>>>>>>> This patch solves the problem by deleting the original node after
>>>>>>>> moving the file. Then the file watcher raises an event whose handler
>>>>>>>> constructs a new node in the new location.
>>>>>>>>
>>>>>>>> This issue comes from the fix to this bug:
>>>>>>>> https://bugs.launchpad.net/kicad/+bug/1852431
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Mikołaj Wielgus
>>>>>>>> <0001-Do-not-reconstruct-proj-tree-on-rename-to-diff-dir.patch>
>>>>>>>> _______________________________________________
>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>
>>>>>>>
>>>>> <0002-Fix-renaming-of-files-in-the-project-tree.patch>
>>> <0001-Revert-changes-in-the-project-manager-file-tree-for-.patch>
>>>
>>>
>>>