#2811 FileFiltersDlg

open
nobody
GUI (476)
3
2009-02-24
2009-02-21
Matthias
No

If you add a new filefilter, or you modify one (name, discription), you can't see what you had changed.
there had been two problems.
For editing: in OnFiltersEditbtn() a refresh had mean misst.
For both, WM was comeing back from editor without waiting for closeing the file.
I added the refresh,OnFiltersEditbtn(),
and changed to modal for the editor by using WaitForSingleObject(processInfo.hProcess, INFINITE);

Discussion

  • Matthias
    Matthias
    2009-02-21

    FileFiltersDlg

     
    Attachments
  • Kimmo Varis
    Kimmo Varis
    2009-02-22

    > I added the refresh,OnFiltersEditbtn()

    This loses the selected item. Save the selected item and re-select it after refreshing the list.

    > For both, WM was comeing back from editor without
    > waiting for closeing the file.

    This is intentional behavior and I won't change it. In some old versions years ago we waited for file close but it was a real pain to use. We cannot block WinMerge when notepad (or other) editor is open.

    The real solution would be to make WinMerge detect file changes from filesystem. This is trivial with modern Windows versions, but tricky to implement for old Windows versions.

     
  • Matthias
    Matthias
    2009-02-22

    FileFiltersDlg2.patch

     
    Attachments
  • Matthias
    Matthias
    2009-02-22

    ok, no problem, so the user has to click twice on edit to get the correct name and discription.

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-22

    Why?

     
  • Matthias
    Matthias
    2009-02-22

    Without waiting for closeing!
    WM is decoding the old contend of the filterfile.
    So we see the old info in our Dlg. also the filter, till we reload same.

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-22

    Or user just closes the dialog and opens it again.

    Waiting for process end is really not an option. I forgot one reason below - user might have editor which supports multiple windows. So if the opened filter is just one window in the editor WinMerge is blocked until the whole editor is closed. Which is simply unacceptable behavior.

    Hmm. Your patch restores the selected filter in the refresh. But that does not work as we haven't yet set the selected filter. Ok, perhaps this is about terminology. Restore the selected item in the list. So that if user has third item selected before refresh, the third item is selected also after the refresh. And remember to check that there still is three items in the list...

     
  • Matthias
    Matthias
    2009-02-22

    or press test, after we also put there a refresh.:)

    >Which is simply unacceptable behavior
    Yes, you mean it's easy to check for the file is closed?
    But I have no ideas to do.

    >we haven't yet set the selected filter
    done in new patch. Or did I fail?

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-22

    > >Which is simply unacceptable behavior
    > Yes, you mean it's easy to check for the file is closed?
    > But I have no ideas to do.

    I just said we cannot wait for the editor process to close.

    If you want to look at how to get file system change events, check the ReadDirectoryChangesW() WinAPI function. And yes, it requires Windows 2000.
    http://msdn.microsoft.com/en-us/library/aa365465\(VS.85).aspx

    > > we haven't yet set the selected filter
    > done in new patch. Or did I fail?

    +// Remove all from filterslist and re-add so we can update UI
    +CString selected;
    +m_Filters->RemoveAll();
    +theApp.m_globalFileFilter.LoadAllFileFilters();
    +theApp.m_globalFileFilter.GetFileFilters(m_Filters, selected);
    +
    +UpdateFiltersList();
    +SelectFilterByIndex(sel);

    If I have third filter selected before this code. I won't be restored as selected item in the list. You just empty the list and restore the item that was selected when the dialog was opened. Not what the user had selected in the dialog before the list was closed.

     
  • Matthias
    Matthias
    2009-02-23

    >I just said we cannot wait for the editor process to close.
    that I had understand.
    Do you wont me to do ReadDirectoryChangesW()
    I'm not shure I can do,min it will take time.

    >Not what the user had selected in the dialog before the list was closed
    ?? the list is sorted, so there will be no change.
    so SelectFilterByIndex(sel) is setting back correct.

    using your example user edits filter3 and still this is selected, what's wrong?
    If he want's to leave, than filter3 gets aktiv.

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-23

    > Do you wont me to do ReadDirectoryChangesW()
    As I wrote, it requires Windows 2000, and WinMerge still supports older Windows versions. So it would be hard to do.

    And if somebody starts working with adding support for detecting file system changes it must be done to support all WinMerge features, folder compare etc. So it is a lot of work. One way would be to load needed APIs dynamically and use them only on modern Windows versions.

    > ?? the list is sorted, so there will be no change.
    Did you consider the possibility user deleted filter file or added new filters while the dialog was open... Every time you read the filter list from "system" you may get different list. Even if the time between reads is one second.

    With GUI we must remember to think about all possibilities user could do...

     
  • Matthias
    Matthias
    2009-02-23

    So I won't do it now. It realy seams to be to hard for me.

    > ?? the list is sorted, so there will be no change.
    I think here you are a little too fast for WM.
    If we go on OnFiltersEditbtn(), we come directly back, so nothing can be deleted or added!
    As there no other task can running between beginn and end.
    With filechange API yes, here we have to be careful, here it can happen.

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-23

    I think I've misunderstood the patch.

    Now after looking at it more carefully and doing some testing I'm still unsure what this patch really tries to do? What it helps to re-read filter list *after* one filter has been opened to editor?

    It may update some descriptions and event find new filters and so on. But is that what users expects. I think it just confuses users when new filters appear and existing filters change when user presses *edit* button for one filter. This code should have its own 'Refresh' button.

     
  • Matthias
    Matthias
    2009-02-23

    Yes, but now you got it.

    you may create the refresh button, I can do the Rest.
    Or just take the editfunction without edit. That's all.

    void FileFiltersDlg::OnRefreshtbtn()
    {
    int sel =- 1;

    sel = m\_listFilters.GetNextItem\(sel, LVNI\_SELECTED\);
    
    // Can't edit first "None"
    if \(sel > 0\)
    \{
    
        // Remove all from filterslist and re-add so we can update UI
        CString selected;
        m\_Filters->RemoveAll\(\);
        theApp.m\_globalFileFilter.LoadAllFileFilters\(\);
        theApp.m\_globalFileFilter.GetFileFilters\(m\_Filters, selected\);
    
        UpdateFiltersList\(\);
        SelectFilterByIndex\(sel\);
    \}
    

    }

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-24

    Hmm. I'm not sure I see the point of the Refresh button. When user really needs it? That is, when user knows to use that button? I don't want add buttons that don't get used.

    Perhaps the better option is to leave the dialog as is and add this refreshing once somebody implements file system changes listening. Lowering also the priority.

     
  • Kimmo Varis
    Kimmo Varis
    2009-02-24

    • priority: 5 --> 3
     
  • Matthias
    Matthias
    2009-02-25

    Let's take a sample.
    You create a new filefilter.
    You change:
    name: Name of filter => Filter one
    desc: Longer description. => new filter

    What di you see:
    Name of filter Longer description.

    now presss refresh button.
    Filter one new filter

    that's all. Ok same come after closeing and reopen that control.
    With my patch he will also see it correct. Even he didn't change the a header again.:=)

    Ok, it's your decision.
    If you wont to keep it, ok. Than close this item.