Customisation

[nbelzer]

Resolves #142

As specified in #172 I started bringing some of my work on Vimari from my own fork into the main repo. This PR is the first of these, it builds upon the work of @nieldm to bring customisation back to Vimari.

Changes

Instead of creating a PR from my own fork (which mainly consists of one branch with all the features I wanted to see) I decided to take a fresh look on the changes by nieldm, fix some of the bugs and clear up some of the related code:

• Removed some of the legacy code that was related to the now legacy configuration handling (global.js and some functions in injected.js).

• Remove closeTabReverse as it was no longer supported through the Safari Extension. This could be added back in, but seemed like something for a separate pull request.

• Update the layout from being extremely vertical (see Enabled option to edit configuration file #163) to a more compact two-column layout:

• Fix an issue where you were not able to switch tabs when the active tab is pinned.
  This was introduced by the Vimari Extension handling tab switching. This code tried to find the active window from the current tab, however pinned tabs do not have a specific window as they appear in all Safari windows. Therefore the code returned nil and therefore tab switching did not work. I was able to resolve this behaviour by using the activeWindow as default value in case we are not able to retrieve the window for a page: (see this extract from SafariExtensionHandler.swift)

/**
 Returns the containing window of a SFSafariPage, if not available default to the current active window.
 */
private func currentWindow(from page: SFSafariPage, completionHandler: @escaping ((SFSafariWindow?) -> Void)) {
    page.getContainingTab() { $0.getContainingWindow() { window in
        if window != nil {
            return completionHandler(window)
        } else {
            SFSafariApplication.getActiveWindow() { window in
                return completionHandler(window)
            }
        }
    }}
}

• Add configuration option for url on new tab.
  As a new tab need to be opened through the Vimari Extension a url needs to be provided. I experimented with using topsites.com and topsites:// to open the default Safari start page, however the .openTab function does not see those as valid urls. Since this PR introduces configuration back into Vimari I introduced a openTabUrl setting that can be set by the user.

Configuration

So currently the default configuration (if it does not exist) will be placed in ~/Library/Containers/net.televator.Vimari.SafariExtension/Data/. This is the location we get when using the following code:

FileManager.default.url(for: .documentDirectory, in: .userDomainMask, appropriateFor: nil, create: false)

I'm not sure if this is the correct way to go about it. When using the .applicationSupportDirectory instead of .documentDirectory in our call to FileManager.default.url the url becomes ~/Library/Containers/net.televator.Vimari.SafariExtension/Data/Library/ which leads me to believe that both are good choices as they seem to be placed in some sort of container specifically for Vimari. I was not able to find anything specific on developer.apple.com except for hig/macos/architecture/preferences, however the page does not mention a preferred location for configuration files on the os.

The configuration file looks as follows (by default):

{
  "excludedUrls": "",
  "linkHintCharacters": "asdfjklqwerzxc",
  "detectByCursorStyle": false,
  "scrollSize": 50,
  "openTabUrl": "https://duckduckgo.com/",
  "modifier": "",
  "bindings": {
      "hintToggle": "f",
      "newTabHintToggle": "shift+f",
      "scrollUp": "k",
      "scrollDown": "j",
      "scrollLeft": "h",
      "scrollRight": "l",
      "scrollUpHalfPage": "u",
      "scrollDownHalfPage": "d",
      "goToPageTop": "g g",
      "goToPageBottom": "shift+g",
      "goBack": "shift+h",
      "goForward": "shift+l",
      "reload": "r",
      "tabForward": "w",
      "tabBack": "q",
      "closeTab": "x",
      "openTab": "t"
  }
}

Whats next?

I don't think this PR is the final step in adding customisation to Vimari. Currently I see some issues that we should address (perhaps before releasing this to the app store):

• Validation of the configuration
  There are many pitfalls where a user could make a wrong change in the configuration that could potentially break the extension for her. For example not providing a correct url for openTabUrl would disable the newTab functionality (as it fails). More worrying though is if a user misses something crucial to the json structure. In the current implementation this would revert the user back to the default settings without any feedback, likely causing confusion.
  Perhaps YAML could be a better alternative here as it requires less syntax, feedback would be appreciated.
  A longer term goal would be to implement the configuration in the UI of Vimari.App (while keeping the file option for advanced users).

• Future configuration options
  Given the current implementation it might be difficult to add configuration options in the future. Which is something that is already being requested (copy current tab url with 'yy' #173). This would require us to add/remove configuration options from a user customised file. My best guess here is to change the implementation to use the user configuration as overrides to the default one. This would allow us to add new configuration options, which the user could then override, and remove configuration options, by simply only looking for overrides for the default options. All while leaving the user configuration alone. Again feedback would be appreciated.

• Proper insert/normal mode
  Some of the requests for configuring keybindings (like in Add more key bindings + ability to assign own key bindings #171) stem from an overlap with the keybindings present on a given website. In my fork of Vimari I have resolved this by creating a proper separation between insert and normal mode. This will be where I put my focus after this pull request.

• Linting (not related to configurability)
  While making these changes I found there no linting setup for Swift or JavaScript. I think contributors would greatly benefit from a linting setup for both of these languages as it would remove the need to try and replicate the same code style and would enhance readability of the code as all will have a similar structure.

---

My apologies for the somewhat large PR, while summarising my changes above I clearly see how I could have done a better job at creating smaller and more easily reviewable changes. I hope that the explanation given together with the commit messages helps guide you through the changes. For future development I will take this into account, feedback is very welcome!

[danielcompton]

In the current implementation this would revert the user back to the default settings without any feedback, likely causing confusion. Perhaps YAML could be a better alternative here as it requires less syntax, feedback would be appreciated.

I think JSON is fine here, it avoids an extra dependency and I'm not a huge fan of YAML in general. If we can check the JSON before save and avoid saving invalid JSON that should solve most of the issues.

My best guess here is to change the implementation to use the user configuration as overrides to the default one.

Yep, this would be idiomatic behaviour. NSUserDefaults behaves this way, and is likely what we'd want to use long-term.

Proper insert/normal mode

I think this is in progress in #176 now?

Linting

Yep, this seems like a good idea. SwiftFormat and Prettier would be my preferences for formatting. I have a weak preference for ESLint for linting JS, I have no opinions on linting Swift.

I was not able to find anything specific on developer.apple.com except for hig/macos/architecture/preferences, however the page does not mention a preferred location for configuration files on the os.

Generally they would end up in ~/Library/Preferences or the container equivalent, but that would require using NSUserDefaults I think. .applicationSupportDirectory seems slightly more idiomatic as it is not data that the application is responsible for creating or managing (vs something like a photo database).

Zooming out a little bit:

1. I'm unsure whether we'll get any kind of config file based preferences past App Review, though I'm happy to see.

2. The long term future for Vimari is to merge back with Vimium as a Web Extension and to be developed from the Vimium code base. I see value in improving Vimari for people still on older operating systems, but would imagine that it won't make a ton of sense to keep putting lots of effort into Vimari once Big Sur comes out. That is assuming that the Web Extension APIs are a suitable replacement for the current APIs. If not, then it may make sense to invest more effort here.

   I don't want to tell you how to spend your time, I'm happy for you to dive deep into Vimari, I just wanted to tell you my perspective.

[danielcompton]

I can't say I 100% understand everything that is happening here, but I'm happy with the approach you're taking. Merge when you're ready. Thanks for your hard work on this!

[nbelzer]

I think JSON is fine here, it avoids an extra dependency and I'm not a huge fan of YAML in general.

Okay this is fine by me.

If we can check the JSON before save and avoid saving invalid JSON that should solve most of the issues.

Yep, this would be idiomatic behaviour. NSUserDefaults behaves this way, and is likely what we'd want to use long-term.

I'll make an issue for both of these.

I think this is in progress in #176 now?

Yes it is.

Yep, this seems like a good idea. SwiftFormat and Prettier would be my preferences for formatting. I have a weak preference for ESLint for linting JS, I have no opinions on linting Swift.

SwiftFormat and Prettier both provide some default configuration we could use of the box. Prettier also seems to have an experimental plugin for Swift but I think we should stick with SwiftFormat instead. I'll make an issue out of this.

The long term future for Vimari is to merge back with Vimium as a Web Extension and to be developed from the Vimium code base. I see value in improving Vimari for people still on older operating systems, but would imagine that it won't make a ton of sense to keep putting lots of effort into Vimari once Big Sur comes out. That is assuming that the Web Extension APIs are a suitable replacement for the current APIs. If not, then it may make sense to invest more effort here.

These changes: configuration (this issue), isolation (#176), smooth scrolling and possibly a help mode would bring Vimari to a good point. From there it would be interesting to see how far we can get with Vimium using WebExtensions for Safari. If I find the time this weekend I might have another go at trying to get Vimium working (for those interested: philc/vimium#3610).

[nchase]

Has this been submitted for review? Would love to be able to use it!

[paulofaria]

From there it would be interesting to see how far we can get with Vimium using WebExtensions for Safari. If I find the time this weekend I might have another go at trying to get Vimium working (for those interested: philc/vimium#3610).
@nbelzer

Any luck on the Vimium port to WebExtensions? If you need any help, let me know. I'm interested in contributing.