Add Opener Registry #265
Add Opener Registry #265
Conversation
There was a problem hiding this comment.
Overall this is looking great! 💖
Not loving the latex.opener config stuff, since it essentially invalidates the "dependency injection" system based around loading openers from the openers directory... but I cannot think of a better alternative right now that still results in giving the user a list of options in the settings view. So let's leave that as is for the time being, unless you have some ideas.
Would also be great if the getName functions could be replaced with a convention based scheme, like simply grabbing the name from the file name sans opener suffix. The functions feel unnecessary since the name is already available elsewhere.
| this.createOpeners() | ||
| } | ||
|
|
||
| async createOpeners () { |
There was a problem hiding this comment.
Maybe rename this to initializeOpeners or some such.
| return new OpenerImpl() | ||
| }) | ||
| // Sort the openers into decreasing priority | ||
| this.openers = _.orderBy(this.openers, [opener => opener.hasSynctex(), opener => opener.canOpenInBackground()], ['desc', 'desc']) |
There was a problem hiding this comment.
Extract this "capability list" into a separate variable for readability? Call it capabilityPrioritization or something along those lines. Maybe simply capabilities?
| const openResultInBackground = atom.config.get('latex.openResultInBackground') | ||
| const enableSynctex = atom.config.get('latex.enableSynctex') | ||
|
|
||
| let openers = _.filter(this.openers, opener => opener.canOpen(filePath)) |
There was a problem hiding this comment.
const openers = this.openers.filter(opener => opener.canOpen(filePath))
Also, I'd probably name it candidates instead of openers to indicate this is the list of candidates from which we will select an opener.
| let opener | ||
|
|
||
| if (name !== 'automatic') { | ||
| opener = _.find(openers, opener => opener.getName() === name) |
There was a problem hiding this comment.
opener = openers.find(opener => opener.getName() === name)
|
One solution to the const schema = atom.config.getSchema('latex.opener')
const dir = path.join(__dirname, 'openers')
const ext = '.js'
this.openers = new Map()
for (const name of schema.enum) {
const OpenerImpl = require(path.format({ dir, name, ext }))
this.openers.set(name, new OpenerImpl())
}The selection logic would then be:
|
|
I like that idea! |
Also simplified output selection logic in Builder.
|
@thomasjo Can you give it one last review? I have added specs for |
| export default class OpenerRegistry { | ||
| openers = new Map() | ||
|
|
||
| async initializeOpeners () { |
There was a problem hiding this comment.
Left over from when I had filtering code there.
| @@ -60,6 +60,7 @@ export default { | |||
| const Composer = require('./composer') | |||
|
|
|||
| global.latex = new Latex() | |||
| latex.opener.initializeOpeners() | |||
There was a problem hiding this comment.
Can we move this to the Latex constructor if/when we make initializeOpeners synchronous? Feels like some responsibility has leaked out here — this needs to be done either in Latex or OpenerRegistry.
There was a problem hiding this comment.
It was done in OpenerRegistry but that caused problems in the spec since the schema was not available.
There was a problem hiding this comment.
Then I propose we fix the spec instead. This just doesn't feel right 💭
There was a problem hiding this comment.
Ok. I'll have to dig into the package activation issue to make it happen. OpenerRegistry is getting created in a whole bunch of places in the specs. Moving it back won't cause spec failure, just console messages, BTW.
|
@thomasjo I've tried migrated the specs to use package activation by making the most minimal changes to specs unrelated to this PR. Looks like everything is working aside from latexmk 4.48 being broken on MiKTeX. I think we should do further tweaks to the unrelated specs if needed in #224 to avoid too many merge conflicts. In other words, I think this is basically ready to 🚢! |
This is PR implements a simple opener registry which tries to pick an opener based on availability and features. The user can override the automatic selection via the
latex.openersetting.Algorithm
latex.openerand that one can open the file then use it.latex.enableSynctexis enabled then use the viewer with the highest priority that supports SyncTeX if one is present. If there are openers that support both SyncTeX and "open in background" then these will be at the front of the list because of the priority sort.latex.openResultInBackgroundis enabled then use the viewer with the highest priority that supports opening in background if one is present.Issues
latex.alwaysOpenResultInAtomsetting tolatex.opener = 'pdf-view'setting.okular --versionif we replaced thelatex.texPathwithlatex.paths. Would make all the opener path settings unneeded and make it possible to find 64bit SumatraPDF without user configuration. Maybe laterEvinceOpenerto move methods into class.Resolves #235 and resolves #85 and resolves #252.