Loading ...
Sorry, an error occurred while loading the content.

Re: Patch to add a Recent Files menu to MacVim

Expand Messages
  • björn
    ... The problem with your suggestion is that MacVim needs to coordinate the recent files from each Vim process, which may be a bit tricky. (Each Vim process
    Message 1 of 10 , May 4, 2008
    • 0 Attachment
      2008/5/4 Ben Schmidt <mail_ben_schmidt@...>:
      >
      > Nico Weber wrote:
      > > Hi,
      > >
      > > the attached patch adds a Recent Files menu to MacVim. For now, only
      > > files opened via File->Open show up.
      >
      > Shouldn't this integrate with some (existing or new) recent files Vim plugin, and
      > just provide GUI access to the same datastore that plugin uses? Or for that
      > matter, since such plugins usually add the recent files to a menu anyway, IIRC,
      > actually need no MacVim specific implementation at all?
      >
      > Not that I'm opposed to a recent files menu, but shouldn't this be done the Vim
      > way rather than a MacVim way?

      The problem with your suggestion is that MacVim needs to coordinate
      the "recent files" from each Vim process, which may be a bit tricky.
      (Each Vim process has its own menu so it would overwrite any "recent
      files" menu of another process.) For this reason and since "recent
      files" isn't a standard item in the other GUI ports anyway (but it
      _is_ a standard for OS X apps), I don't think Nico's approach is all
      that bad.

      Nico, thanks for the patch. I haven't had a chance to look at it yet,
      but I'll let you know when I have.

      /Björn

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_mac" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Nico Weber
      ... That s not mutually exclusive in my eyes. If you prefer a Vim recent files plugin, you can use it. If you want the GUI recent files menu, it s now there,
      Message 2 of 10 , May 4, 2008
      • 0 Attachment
        > Not that I'm opposed to a recent files menu, but shouldn't this be
        > done the Vim
        > way rather than a MacVim way?

        That's not mutually exclusive in my eyes. If you prefer a Vim recent
        files plugin, you can use it. If you want the GUI recent files menu,
        it's now there, too. Note that the patch also adds the files you open
        with File->Open to the global Recent Items menu (which you can access
        by clicking on the apple in the upper left corner of your screen) --
        that's not possible in a Vim-only way (well, we could a vimscript
        function addrecentfile() that does this on "systems that support a
        recent file list", a "clearrecentfiles()" function, a vimscript list
        "recentfiles" that returns the recent file list provided by the
        system, etc and then somehow implement this in vimscript. But that's
        like 5 times as much code, and I believe a Recent Files menu is only
        customary on OS X anyways).

        >

        I guess I just want to say that I won't write a vimscript version of
        this patch. If someone else provides a vimscript solution that works
        just as well (where "just as well" means "nearly exactly like the
        usual Recent Files menu on OS X"), I wouldn't mind if that patch was
        included instead :-)

        Nico


        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_mac" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • björn
        Nico, I have looked over the patch a bit and made some minor changes (attached). I m a bit confused about the memory handling. As you ll see in the patch I m
        Message 3 of 10 , May 9, 2008
        • 0 Attachment
          Nico,

          I have looked over the patch a bit and made some minor changes
          (attached). I'm a bit confused about the memory handling. As you'll
          see in the patch I'm releasing the "recentFilesMenu" since this was a
          memory leak. However, I'm not sure if you were assuming it was not
          released when you call replace:with: (I also changed the name of that
          message for now...it probably should be moved as well.)? I'm not sure
          something might go wrong when you do the swapping of menu items since
          one item is released (the one that is swapped out)...it seems a bit
          precarious to me. Have you thought about it?

          Also, it makes more sense to me to add files to the recent files menu
          in application:openFiles: instead of in fileOpen:. This way, files
          that are double-clicked, dragged to the dock icon, etc. will also be
          put on the recent files menu (and not only files that are opened via
          Cmd-o). However, I make sure files that are being edited remotely
          aren't added to the recent files menu.

          Finally, I am trying to come up with a better way to deal with menus
          so that we don't have to do these kind of hacks just to get the recent
          files menu added, so somewhere down the track we will hopefully be
          able to clean it up a bit.


          Björn

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_mac" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • Nico Weber
          Hi, ... Looks good to me. ... Feel free to move it anywhere you want :-P ... In the first call to replaceMenuItem:, the item that s swapped out is
          Message 4 of 10 , May 9, 2008
          • 0 Attachment
            Hi,

            > I have looked over the patch a bit and made some minor changes
            > (attached).

            Looks good to me.

            > I'm a bit confused about the memory handling. As you'll
            > see in the patch I'm releasing the "recentFilesMenu" since this was a
            > memory leak. However, I'm not sure if you were assuming it was not
            > released when you call replace:with: (I also changed the name of that
            > message for now...it probably should be moved as well.)?

            Feel free to move it anywhere you want :-P

            > I'm not sure
            > something might go wrong when you do the swapping of menu items since
            > one item is released (the one that is swapped out)...it seems a bit
            > precarious to me. Have you thought about it?

            In the first call to replaceMenuItem:, the item that's swapped out is
            recentFilesMenuItem, which is retained by all VimControllers (they all
            share just one recentFilesMenuItem, doesn't work otherwise) and by the
            AppController, so this is ok. In the second call, the first argument
            it the recentFilesDummy, which should be retained recentFilesMenuItem,
            because [recentFilesMenuItem setRepresentedObject:] is called before.

            And when setRepresentedObject: is called later again and another
            representedObject is set, the old representedObject is swapped in.

            So, if a MenuItem retains its representedObject, this should work.

            > Also, it makes more sense to me to add files to the recent files menu
            > in application:openFiles: instead of in fileOpen:. This way, files
            > that are double-clicked, dragged to the dock icon, etc. will also be
            > put on the recent files menu (and not only files that are opened via
            > Cmd-o). However, I make sure files that are being edited remotely
            > aren't added to the recent files menu.

            Ok.

            > Finally, I am trying to come up with a better way to deal with menus
            > so that we don't have to do these kind of hacks just to get the recent
            > files menu added, so somewhere down the track we will hopefully be
            > able to clean it up a bit.

            Sounds good :-)

            Nico

            --~--~---------~--~----~------------~-------~--~----~
            You received this message from the "vim_mac" maillist.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          • björn
            ... Ok, but shouldn t the recentFilesDummy still be retained in addMenuItemWithTag and then released in dealloc? Or is there a reason why you don t retain it?
            Message 5 of 10 , May 9, 2008
            • 0 Attachment
              2008/5/9 Nico Weber <nicolasweber@...>:
              >
              >> I'm not sure
              >> something might go wrong when you do the swapping of menu items since
              >> one item is released (the one that is swapped out)...it seems a bit
              >> precarious to me. Have you thought about it?
              >
              > In the first call to replaceMenuItem:, the item that's swapped out is
              > recentFilesMenuItem, which is retained by all VimControllers (they all
              > share just one recentFilesMenuItem, doesn't work otherwise) and by the
              > AppController, so this is ok. In the second call, the first argument
              > it the recentFilesDummy, which should be retained recentFilesMenuItem,
              > because [recentFilesMenuItem setRepresentedObject:] is called before.
              >
              > And when setRepresentedObject: is called later again and another
              > representedObject is set, the old representedObject is swapped in.
              >
              > So, if a MenuItem retains its representedObject, this should work.

              Ok, but shouldn't the recentFilesDummy still be retained in
              addMenuItemWithTag and then released in dealloc? Or is there a reason
              why you don't retain it?

              Björn

              --~--~---------~--~----~------------~-------~--~----~
              You received this message from the "vim_mac" maillist.
              For more information, visit http://www.vim.org/maillist.php
              -~----------~----~----~----~------~----~------~--~---
            • Nico Weber
              ... Retaining it once more doesn t hurt, and it s probably clearer. ... No…I just figured that some other object always retains the dummy item, so this is
              Message 6 of 10 , May 9, 2008
              • 0 Attachment
                > Ok, but shouldn't the recentFilesDummy still be retained in
                > addMenuItemWithTag and then released in dealloc?

                Retaining it once more doesn't hurt, and it's probably clearer.

                > Or is there a reason why you don't retain it?

                No…I just figured that some other object always retains the dummy
                item, so this is not necessary. But retaining it one more time is more
                explicit and less brittle, so that's fine with me too ;-)

                Thanks,
                Nico


                --~--~---------~--~----~------------~-------~--~----~
                You received this message from the "vim_mac" maillist.
                For more information, visit http://www.vim.org/maillist.php
                -~----------~----~----~----~------~----~------~--~---
              • Nico Weber
                ... Thanks for merging! :-) Perhaps the Open Recent should be moved two items down though, so that it s directly above the separator. This is were Open
                Message 7 of 10 , May 9, 2008
                • 0 Attachment
                  >> Ok, but shouldn't the recentFilesDummy still be retained in
                  >> addMenuItemWithTag and then released in dealloc?
                  >
                  > Retaining it once more doesn't hurt, and it's probably clearer.
                  >
                  >> Or is there a reason why you don't retain it?
                  >
                  > No…I just figured that some other object always retains the dummy
                  > item, so this is not necessary. But retaining it one more time is more
                  > explicit and less brittle, so that's fine with me too ;-)

                  Thanks for merging! :-)

                  Perhaps the "Open Recent" should be moved two items down though, so
                  that it's directly above the separator. This is were Open Recent is in
                  all other apps I just checked (the other apps don't have quite as many
                  "open" menu items as MacVim though). It looks nicer, too.

                  Patch (likely overkill :-P):

                  diff --git a/src/MacVim/gvimrc b/src/MacVim/gvimrc
                  index ca9910c..d0db066 100644
                  --- a/src/MacVim/gvimrc
                  +++ b/src/MacVim/gvimrc
                  @@ -45,7 +45,7 @@ aunmenu File.Save-Exit
                  an <silent> 10.290 File.New\ Window :maca newWindow:<CR>
                  an 10.295 File.New\ Tab :tabnew<CR>
                  an <silent> 10.310 File.&Open\.\.\. :maca fileOpen:<CR>
                  -an <silent> 10.314 File.Open\ Recent :maca
                  recentFilesDummy:<CR>
                  +an <silent> 10.327 File.Open\ Recent :maca
                  recentFilesDummy:<CR>
                  an 10.328 File.-SEP0- <Nop>
                  an <silent> 10.330 File.Close\ Window<Tab>:qa :confirm qa<CR>
                  an <silent> 10.331 File.Close :maca
                  performClose:<CR>

                  Thanks,
                  Nico
                  --~--~---------~--~----~------------~-------~--~----~
                  You received this message from the "vim_mac" maillist.
                  For more information, visit http://www.vim.org/maillist.php
                  -~----------~----~----~----~------~----~------~--~---
                • björn
                  ... Ok. I ve moved it downwards now. I also pushed a patch which ensures that files that are opened/saved from a :browse command are added to the Recent
                  Message 8 of 10 , May 10, 2008
                  • 0 Attachment
                    2008/5/9 Nico Weber <nicolasweber@...>:
                    >
                    > Perhaps the "Open Recent" should be moved two items down though, so
                    > that it's directly above the separator. This is were Open Recent is in
                    > all other apps I just checked (the other apps don't have quite as many
                    > "open" menu items as MacVim though). It looks nicer, too.

                    Ok. I've moved it downwards now. I also pushed a patch which ensures
                    that files that are opened/saved from a :browse command are added to
                    the "Recent Files" menu. It got kind of confusing otherwise since
                    "Open..." would put a file on the "Recent Files" menu but
                    "Split-Open..." and "Open Tab..." wouldn't. Well, now they do.

                    Björn

                    --~--~---------~--~----~------------~-------~--~----~
                    You received this message from the "vim_mac" maillist.
                    For more information, visit http://www.vim.org/maillist.php
                    -~----------~----~----~----~------~----~------~--~---
                  Your message has been successfully submitted and would be delivered to recipients shortly.