New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consume File-Icons' element-icon service #870

Merged
merged 15 commits into from Nov 1, 2017

Conversation

Projects
None yet
3 participants
@Alhadis
Contributor

Alhadis commented Mar 9, 2017

This PR applies to the tabs package what's been described in atom/tree-view#1146 for the tree-view package. The PR's updated wording describes this scenario best, so I'll just copy+paste:


I realised too late that your icon-service fulfils a different purpose from what our package needed. We wrote our own instead, which is built on a dubious but (currently) stable foundation of monkey-patches.

Opinions differ on the subject, but mine is that monkey-patching code you didn't write is bad, and should only be used as a last resort (DOM polyfills notwithstanding). Which is precisely what I've been forced to do to get dynamic icon-assignment working.

This needs to be fixed at a formal level, because:

  1. Patches don't persist if File-Icons is deactivated and reactivated.
    I've actually chosen not to fix this, because it would step outside the expected lifecycle of the package. E.g., when a user deactivates a package, they expect it to leave no traces in the workspace. I also can't imagine this happening too often, but still...

  2. Any innocent change to your packages could break stuff.
    The obvious danger of monkey-patching what wasn't expected or supposed to be changed. We have specs to alert us of breakage, but there's no telling what would be involved in the repair. We both know this is the wrong approach.

Now, I don't know how the Atom team would feel about adding support for third-party package services. Ideally, this would be addressed on the level of Atom's core icon-API. However, the differences of our needs (as well as the specific use-case of our needs) make me hesitant to propose a change to your existing service, especially because @as-cii has stressed he prefers keeping its functionality simple for the time being. In light of that, it would be more appropriate and ergonomic to support a third-party service in the interim, should a mutually-compatible solution be realised in future.

What this service does

The file-icons.element-icons service, when consumed, provides a function to add dynamic icons to DOM elements. The function is to be called by the package on any element that's supposed to represent a filesystem resource (either files, or directories):

let fileIcon = document.querySelector("li.file-entry > span.icon");
addIconToElement(fileIcon, "/path/to/file.txt");

Calling the function returns a Disposable that clears the icon-node instance from memory, which should be done once the view is destroyed.

Note there's no requirement to specify whether a path is a file or directory. Our heavy-duty filesystem API takes care of the heavy lifting... I've even plans to separate it from File-Icons in the form of a standalone Node module, so other package authors can benefit from my hard work too. :)

Related pull-requests

@50Wliu 50Wliu added the needs-review label Mar 10, 2017

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jul 19, 2017

Contributor

@50Wliu I'm keeping this package synced with upstream changes so users have a chance to test my fork using the latest version of the package. I strongly recommend squash-merging when (or if...) this gets considered.

Contributor

Alhadis commented Jul 19, 2017

@50Wliu I'm keeping this package synced with upstream changes so users have a chance to test my fork using the latest version of the package. I strongly recommend squash-merging when (or if...) this gets considered.

@nathansobo nathansobo self-assigned this Oct 30, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Same with this one. Would you be cool adding a basic test where we interact with the service and don't render the default icons?

Contributor

nathansobo commented Oct 31, 2017

Same with this one. Would you be cool adding a basic test where we interact with the service and don't render the default icons?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

Oh, I've nearly finished these. :D I'm extending the current icon-related specs in results-view-spec.js.

Contributor

Alhadis commented Oct 31, 2017

Oh, I've nearly finished these. :D I'm extending the current icon-related specs in results-view-spec.js.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

@nathansobo, @50Wliu ... Could one of you please help me? I need assistance figuring out Jasmine's retarded Promise-handling because AFAIK, this shit shouldn't be happening:

Figure 1

I need to delay the next bunch of assertions for the element-icons service, because they modify the DOM on the next tick (in order to get around the "virtual DOM" being used by the ProjectFindView class. So a small superficial timeout is all that's needed to reliably assert the working interface, but I can't figure this shit out. I'm about to lose whatever remained of sanity.

  ███████╗██╗    ██╗██╗████████╗ ██████╗██╗  ██╗    ████████╗ ██████╗          
  ██╔════╝██║    ██║██║╚══██╔══╝██╔════╝██║  ██║    ╚══██╔══╝██╔═══██╗         
  ███████╗██║ █╗ ██║██║   ██║   ██║     ███████║       ██║   ██║   ██║         
  ╚════██║██║███╗██║██║   ██║   ██║     ██╔══██║       ██║   ██║   ██║         
  ███████║╚███╔███╔╝██║   ██║   ╚██████╗██║  ██║       ██║   ╚██████╔╝         
  ╚══════╝ ╚══╝╚══╝ ╚═╝   ╚═╝    ╚═════╝╚═╝  ╚═╝       ╚═╝    ╚═════╝          
                                                                               
       ███╗   ███╗    ██████╗      ██████╗    ██╗  ██╗     █████╗              
       ████╗ ████║   ██╔═══██╗    ██╔════╝    ██║  ██║    ██╔══██╗             
       ██╔████╔██║   ██║   ██║    ██║         ███████║    ███████║             
       ██║╚██╔╝██║   ██║   ██║    ██║         ██╔══██║    ██╔══██║             
       ██║ ╚═╝ ██║   ╚██████╔╝    ╚██████╗    ██║  ██║    ██║  ██║             
       ╚═╝     ╚═╝    ╚═════╝      ╚═════╝    ╚═╝  ╚═╝    ╚═╝  ╚═╝             
                                                                               
   █████╗   ██╗       ██████╗    ███████╗    █████╗    ██████╗   ██╗   ██╗
  ██╔══██╗  ██║       ██╔══██╗   ██╔════╝   ██╔══██╗   ██╔══██╗  ╚██╗ ██╔╝
  ███████║  ██║       ██████╔╝   █████╗     ███████║   ██║  ██║   ╚████╔╝ 
  ██╔══██║  ██║       ██╔══██╗   ██╔══╝     ██╔══██║   ██║  ██║    ╚██╔╝  
  ██║  ██║  ███████╗  ██║  ██║   ███████╗   ██║  ██║   ██████╔╝     ██║   
  ╚═╝  ╚═╝  ╚══════╝  ╚═╝  ╚═╝   ╚══════╝   ╚═╝  ╚═╝   ╚═════╝      ╚═╝   

░▒███████                   ░▒███████                  ░▒███████                
░██▓▒░░▒▓██                 ░██▓▒░░▒▓██                ░██▓▒░░▒▓██              
██▓▒░__░▒▓██___██████       ██▓▒░__░▒▓██___██████      ██▓▒░__░▒▓██___██████    
██▓▒░____░▓███▓__░▒▓██      ██▓▒░____░▓███▓__░▒▓██     ██▓▒░____░▓███▓__░▒▓██   
██▓▒░___░▓██▓_____░▒▓██     ██▓▒░___░▓██▓_____░▒▓██    ██▓▒░___░▓██▓_____░▒▓██  
██▓▒░_______________░▒▓██   ██▓▒░_______________░▒▓██  ██▓▒░_______________░▒▓██
_██▓▒░______________░▒▓██   _██▓▒░______________░▒▓██  _██▓▒░______________░▒▓██
__██▓▒░____________░▒▓██    __██▓▒░____________░▒▓██   __██▓▒░____________░▒▓██ 
___██▓▒░__________░▒▓██     ___██▓▒░__________░▒▓██    ___██▓▒░__________░▒▓██  
____██▓▒░________░▒▓██      ____██▓▒░________░▒▓██     ____██▓▒░________░▒▓██   
_____██▓▒░_____░▒▓██        _____██▓▒░_____░▒▓██       _____██▓▒░_____░▒▓██     
______██▓▒░__░▒▓██          ______██▓▒░__░▒▓██         ______██▓▒░__░▒▓██       
_______█▓▒░░▒▓██            _______█▓▒░░▒▓██           _______█▓▒░░▒▓██         
_________░▒▓██              _________░▒▓██             _________░▒▓██           
_______░▒▓██                _______░▒▓██               _______░▒▓██             
_____░▒▓██                  _____░▒▓██                 _____░▒▓██                                                                                           
Contributor

Alhadis commented Nov 1, 2017

@nathansobo, @50Wliu ... Could one of you please help me? I need assistance figuring out Jasmine's retarded Promise-handling because AFAIK, this shit shouldn't be happening:

Figure 1

I need to delay the next bunch of assertions for the element-icons service, because they modify the DOM on the next tick (in order to get around the "virtual DOM" being used by the ProjectFindView class. So a small superficial timeout is all that's needed to reliably assert the working interface, but I can't figure this shit out. I'm about to lose whatever remained of sanity.

  ███████╗██╗    ██╗██╗████████╗ ██████╗██╗  ██╗    ████████╗ ██████╗          
  ██╔════╝██║    ██║██║╚══██╔══╝██╔════╝██║  ██║    ╚══██╔══╝██╔═══██╗         
  ███████╗██║ █╗ ██║██║   ██║   ██║     ███████║       ██║   ██║   ██║         
  ╚════██║██║███╗██║██║   ██║   ██║     ██╔══██║       ██║   ██║   ██║         
  ███████║╚███╔███╔╝██║   ██║   ╚██████╗██║  ██║       ██║   ╚██████╔╝         
  ╚══════╝ ╚══╝╚══╝ ╚═╝   ╚═╝    ╚═════╝╚═╝  ╚═╝       ╚═╝    ╚═════╝          
                                                                               
       ███╗   ███╗    ██████╗      ██████╗    ██╗  ██╗     █████╗              
       ████╗ ████║   ██╔═══██╗    ██╔════╝    ██║  ██║    ██╔══██╗             
       ██╔████╔██║   ██║   ██║    ██║         ███████║    ███████║             
       ██║╚██╔╝██║   ██║   ██║    ██║         ██╔══██║    ██╔══██║             
       ██║ ╚═╝ ██║   ╚██████╔╝    ╚██████╗    ██║  ██║    ██║  ██║             
       ╚═╝     ╚═╝    ╚═════╝      ╚═════╝    ╚═╝  ╚═╝    ╚═╝  ╚═╝             
                                                                               
   █████╗   ██╗       ██████╗    ███████╗    █████╗    ██████╗   ██╗   ██╗
  ██╔══██╗  ██║       ██╔══██╗   ██╔════╝   ██╔══██╗   ██╔══██╗  ╚██╗ ██╔╝
  ███████║  ██║       ██████╔╝   █████╗     ███████║   ██║  ██║   ╚████╔╝ 
  ██╔══██║  ██║       ██╔══██╗   ██╔══╝     ██╔══██║   ██║  ██║    ╚██╔╝  
  ██║  ██║  ███████╗  ██║  ██║   ███████╗   ██║  ██║   ██████╔╝     ██║   
  ╚═╝  ╚═╝  ╚══════╝  ╚═╝  ╚═╝   ╚══════╝   ╚═╝  ╚═╝   ╚═════╝      ╚═╝   

░▒███████                   ░▒███████                  ░▒███████                
░██▓▒░░▒▓██                 ░██▓▒░░▒▓██                ░██▓▒░░▒▓██              
██▓▒░__░▒▓██___██████       ██▓▒░__░▒▓██___██████      ██▓▒░__░▒▓██___██████    
██▓▒░____░▓███▓__░▒▓██      ██▓▒░____░▓███▓__░▒▓██     ██▓▒░____░▓███▓__░▒▓██   
██▓▒░___░▓██▓_____░▒▓██     ██▓▒░___░▓██▓_____░▒▓██    ██▓▒░___░▓██▓_____░▒▓██  
██▓▒░_______________░▒▓██   ██▓▒░_______________░▒▓██  ██▓▒░_______________░▒▓██
_██▓▒░______________░▒▓██   _██▓▒░______________░▒▓██  _██▓▒░______________░▒▓██
__██▓▒░____________░▒▓██    __██▓▒░____________░▒▓██   __██▓▒░____________░▒▓██ 
___██▓▒░__________░▒▓██     ___██▓▒░__________░▒▓██    ___██▓▒░__________░▒▓██  
____██▓▒░________░▒▓██      ____██▓▒░________░▒▓██     ____██▓▒░________░▒▓██   
_____██▓▒░_____░▒▓██        _____██▓▒░_____░▒▓██       _____██▓▒░_____░▒▓██     
______██▓▒░__░▒▓██          ______██▓▒░__░▒▓██         ______██▓▒░__░▒▓██       
_______█▓▒░░▒▓██            _______█▓▒░░▒▓██           _______█▓▒░░▒▓██         
_________░▒▓██              _________░▒▓██             _________░▒▓██           
_______░▒▓██                _______░▒▓██               _______░▒▓██             
_____░▒▓██                  _____░▒▓██                 _____░▒▓██                                                                                           
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 1, 2017

Member

I'm still trying to figure out when exactly to use this (I was completely off the mark in a previous conversation), but try calling jasmine.useRealClock().

Member

50Wliu commented Nov 1, 2017

I'm still trying to figure out when exactly to use this (I was completely off the mark in a previous conversation), but try calling jasmine.useRealClock().

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

OMFG, that explains it. What the hell, guys, that isn't cool at all. 😢

Okay, yeah, the tampering now explains every timeout-related frustration I've had with Jasmine in the past. I stuck beforeEach(() => jasmine.useRealClock()) at the top of the suite, and now it's failing again for reasons I can at least look into. 👍 Thanks!

Contributor

Alhadis commented Nov 1, 2017

OMFG, that explains it. What the hell, guys, that isn't cool at all. 😢

Okay, yeah, the tampering now explains every timeout-related frustration I've had with Jasmine in the past. I stuck beforeEach(() => jasmine.useRealClock()) at the top of the suite, and now it's failing again for reasons I can at least look into. 👍 Thanks!

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

Done. Took up most of my day. Please tell me there's nothing I missed.

Contributor

Alhadis commented Nov 1, 2017

Done. Took up most of my day. Please tell me there's nothing I missed.

@nathansobo nathansobo merged commit 10808e1 into atom:master Nov 1, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

@Alhadis Published as 0.213.0 and upgraded in atom/atom@afc341e.

I'm really sorry that our legacy spec environment is such a pain. It's a major piece of technical debt that we need to pay off on these original packages. You probably know about atom-mocha-test-runner. We just need to convert these packages.

Contributor

nathansobo commented Nov 1, 2017

@Alhadis Published as 0.213.0 and upgraded in atom/atom@afc341e.

I'm really sorry that our legacy spec environment is such a pain. It's a major piece of technical debt that we need to pay off on these original packages. You probably know about atom-mocha-test-runner. We just need to convert these packages.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

Thanks again for all these contributions. I know it was a lot of work to upstream this stuff but it's so much better than hacking the DOM. Thanks for doing the right thing!

Please ping me on any followup PRs I may have missed around this stuff.

Contributor

nathansobo commented Nov 1, 2017

Thanks again for all these contributions. I know it was a lot of work to upstream this stuff but it's so much better than hacking the DOM. Thanks for doing the right thing!

Please ping me on any followup PRs I may have missed around this stuff.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

t's a major piece of technical debt that we need to pay off on these original packages.

Don't sweat it mate, I know there'd have to be a damn good reason for using legacy Jasmine in an era when Mocha and Chai exist/ :D

You probably know about atom-mocha-test-runner. We just need to convert these packages.

I'd be willing to help, as always. Would this involve me killing more CoffeeScript...? :)

Contributor

Alhadis commented Nov 1, 2017

t's a major piece of technical debt that we need to pay off on these original packages.

Don't sweat it mate, I know there'd have to be a damn good reason for using legacy Jasmine in an era when Mocha and Chai exist/ :D

You probably know about atom-mocha-test-runner. We just need to convert these packages.

I'd be willing to help, as always. Would this involve me killing more CoffeeScript...? :)

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

I'd be willing to help, as always. Would this involve me killing more CoffeeScript...? :)

You can kill as much CoffeeScript as you'd like as far as I'm concerned. The biggest blocker there is just making sure you're not clobbering high-value PRs when you convert stuff.

As for the tests, it would basically involve switching them to atom-mocha-test-runner. We prefer to use the native node assert module instead of Chai these days. It would mostly involve replacing a lot of horrible waitsFor and runs blocks with async/await, troubleshooting assumptions baked into the 1.3 spec environment that aren't present in the runner, etc, etc. But if you'd be willing to help that could be an epic contribution to improving the maintainability of our packages.

Priority-wise it would be best to start with the most important packages like find-and-replace, fuzzy-finder, etc. But it might also make sense to start with smaller, simpler packages to get some momentum and encounter any challenges in a less complex setting.

Contributor

nathansobo commented Nov 1, 2017

I'd be willing to help, as always. Would this involve me killing more CoffeeScript...? :)

You can kill as much CoffeeScript as you'd like as far as I'm concerned. The biggest blocker there is just making sure you're not clobbering high-value PRs when you convert stuff.

As for the tests, it would basically involve switching them to atom-mocha-test-runner. We prefer to use the native node assert module instead of Chai these days. It would mostly involve replacing a lot of horrible waitsFor and runs blocks with async/await, troubleshooting assumptions baked into the 1.3 spec environment that aren't present in the runner, etc, etc. But if you'd be willing to help that could be an epic contribution to improving the maintainability of our packages.

Priority-wise it would be best to start with the most important packages like find-and-replace, fuzzy-finder, etc. But it might also make sense to start with smaller, simpler packages to get some momentum and encounter any challenges in a less complex setting.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

As for the tests, it would basically involve switching them to atom-mocha-test-runner.

It'd be cool if Atom used my Mocha integration, haha. But I understand if you guys prefer the minimalist and traditional dot-based look. =)

<img src="https://raw.githubusercontent.com/Alhadis/Atom-Mocha/master/docs/img/slide.gif" "width="900" />

Contributor

Alhadis commented Nov 1, 2017

As for the tests, it would basically involve switching them to atom-mocha-test-runner.

It'd be cool if Atom used my Mocha integration, haha. But I understand if you guys prefer the minimalist and traditional dot-based look. =)

<img src="https://raw.githubusercontent.com/Alhadis/Atom-Mocha/master/docs/img/slide.gif" "width="900" />

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

@Alhadis In general, I am very concerned about following the "less is more" approach and being judicious about adding complexity and dependencies.

I scoped out your runner and it looks pretty cool, though when I see lots of features I sometimes get anxious. Can you pitch us on how it's different than the runner we're using on other mocha-based packages? What problems does it solve that would warrant the overhead of switching at this point? If you weren't the author, would you still want to use it?

Also, we'd prefer to prevent a profusion of different runners. We already have some packages using the other one. How hard would it be to convert them and how open are you going to be to us being fairly opinionated on how the runner for bundled packages functions.

Contributor

nathansobo commented Nov 1, 2017

@Alhadis In general, I am very concerned about following the "less is more" approach and being judicious about adding complexity and dependencies.

I scoped out your runner and it looks pretty cool, though when I see lots of features I sometimes get anxious. Can you pitch us on how it's different than the runner we're using on other mocha-based packages? What problems does it solve that would warrant the overhead of switching at this point? If you weren't the author, would you still want to use it?

Also, we'd prefer to prevent a profusion of different runners. We already have some packages using the other one. How hard would it be to convert them and how open are you going to be to us being fairly opinionated on how the runner for bundled packages functions.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

though when I see lots of features I sometimes get anxious

Don't worry, the package is in dire need of a large clean-up. Superfluous or pointless/minor features have to go. More emphasis on JS and CSS customisation is needed, rather than adding more and more options. Bear in mind this spec-runner was written during the development of file-icons, so I never had the time to focus on it fully in the past. That's why its feature-set has come out looking a little bit... lopsided...

What problems does it solve that would warrant the overhead of switching at this point? If you weren't the author, would you still want to use it?

The spec-runner is built for speed and clear, direct feedback because users are expected to be irritable and sick of rerunning monolithic spec suites all the time. I was, and I wanted to use --grep and all the other awesome features that Mocha gives you on command-line, yet somehow make them available in an interactive form within Atom. Stuff like rerunning only certain tests is a huge help when the alternative is waiting for ~600 tests to pass so you can see what your next failure looks like. :(

Contributor

Alhadis commented Nov 1, 2017

though when I see lots of features I sometimes get anxious

Don't worry, the package is in dire need of a large clean-up. Superfluous or pointless/minor features have to go. More emphasis on JS and CSS customisation is needed, rather than adding more and more options. Bear in mind this spec-runner was written during the development of file-icons, so I never had the time to focus on it fully in the past. That's why its feature-set has come out looking a little bit... lopsided...

What problems does it solve that would warrant the overhead of switching at this point? If you weren't the author, would you still want to use it?

The spec-runner is built for speed and clear, direct feedback because users are expected to be irritable and sick of rerunning monolithic spec suites all the time. I was, and I wanted to use --grep and all the other awesome features that Mocha gives you on command-line, yet somehow make them available in an interactive form within Atom. Stuff like rerunning only certain tests is a huge help when the alternative is waiting for ~600 tests to pass so you can see what your next failure looks like. :(

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

How hard would it be to convert them and how open are you going to be to us being fairly opinionated on how the runner for bundled packages functions.

The package makes no assumptions about its use case. Everything that's there can be replaced if needed. The default HTML reporter is also available as an option, if somebody doesn't like the terminal look. It's extremely extensible, but my documentation of it is embarrassingly sloppy and needs improvement ... >_>

Contributor

Alhadis commented Nov 1, 2017

How hard would it be to convert them and how open are you going to be to us being fairly opinionated on how the runner for bundled packages functions.

The package makes no assumptions about its use case. Everything that's there can be replaced if needed. The default HTML reporter is also available as an option, if somebody doesn't like the terminal look. It's extremely extensible, but my documentation of it is embarrassingly sloppy and needs improvement ... >_>

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

Cool, well I'd be down to explore it. Getting the tests into Mocha is probably the hardest part, so worst case we could decide to go back to the simpler runner. Or maybe we'll love it.

Contributor

nathansobo commented Nov 1, 2017

Cool, well I'd be down to explore it. Getting the tests into Mocha is probably the hardest part, so worst case we could decide to go back to the simpler runner. Or maybe we'll love it.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

Are there that many projects afflicted from this scourge? :(

Good grief, I'm now thinking about adding a migration feature. Hopefully I won't give into the temptation to write it in Perl. 😀

Contributor

Alhadis commented Nov 1, 2017

Are there that many projects afflicted from this scourge? :(

Good grief, I'm now thinking about adding a migration feature. Hopefully I won't give into the temptation to write it in Perl. 😀

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

Which scourge? "Perl" and "temptation" don't occupy similar areas of my brain. I do like caramel apples though.

Contributor

nathansobo commented Nov 1, 2017

Which scourge? "Perl" and "temptation" don't occupy similar areas of my brain. I do like caramel apples though.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

Perl is my second favourite programming language, and if you thought last year's epic CSS rehaul was anything, you should see what I'm gonna do to our embarrassing Perl 5 grammar. 😂👌

Oh yeah, I write documentation in Roff instead of Markdown, because the shit actually ain't that hard. :D Once Atom has a real-time emulator to browse typeset manual pages, you'll see what I mean. 😀

And I learn some APL. I want to master it before I die.

"Normality is just a cultural construct"

Contributor

Alhadis commented Nov 1, 2017

Perl is my second favourite programming language, and if you thought last year's epic CSS rehaul was anything, you should see what I'm gonna do to our embarrassing Perl 5 grammar. 😂👌

Oh yeah, I write documentation in Roff instead of Markdown, because the shit actually ain't that hard. :D Once Atom has a real-time emulator to browse typeset manual pages, you'll see what I mean. 😀

And I learn some APL. I want to master it before I die.

"Normality is just a cultural construct"

nathansobo added a commit that referenced this pull request Nov 10, 2017

Revert "Merge pull request #870 from Cutlery-Drawer/file-icons"
This reverts commit 10808e1, reversing
changes made to a3b2eba.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

@Alhadis Hate to do this, but I had to revert this. I think we need to spend some more time thinking through the interaction between the imperative and declarative DOM manipulation. On initial review I missed that calling getIconClasses had a side-effect. It's important that we find a way to meet our goals without performing side effects in render. I will stay engaged as we figure this out though admittedly next week will be a bit crazy.

Contributor

nathansobo commented Nov 10, 2017

@Alhadis Hate to do this, but I had to revert this. I think we need to spend some more time thinking through the interaction between the imperative and declarative DOM manipulation. On initial review I missed that calling getIconClasses had a side-effect. It's important that we find a way to meet our goals without performing side effects in render. I will stay engaged as we figure this out though admittedly next week will be a bit crazy.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

By side-effect, you're talking about the iconDisposable, yes?

That could easily be handled by the IconServices class using a Map of Disposables => Etch Views instead. It doesn't need to be stored directly on the Etch view.

Contributor

Alhadis commented Nov 10, 2017

By side-effect, you're talking about the iconDisposable, yes?

That could easily be handled by the IconServices class using a Map of Disposables => Etch Views instead. It doesn't need to be stored directly on the Etch view.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

I think storing the iconDisposable on the result views could be fine. After constructing the result view you could imperatively call into the service with the reference to the icon, which you know will be defined once etch.initailize completes. Then on destruction you could dispose the disposable like you do now. I think this is close it's just that you shouldn't have interacted with the service in render.

Contributor

nathansobo commented Nov 10, 2017

I think storing the iconDisposable on the result views could be fine. After constructing the result view you could imperatively call into the service with the reference to the icon, which you know will be defined once etch.initailize completes. Then on destruction you could dispose the disposable like you do now. I think this is close it's just that you shouldn't have interacted with the service in render.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

Working on it.

Would the didAddResult method in lib/project/result-view.js be suitable for calling the service?

Contributor

Alhadis commented Nov 10, 2017

Working on it.

Would the didAddResult method in lib/project/result-view.js be suitable for calling the service?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

Would the didAddResult method in lib/project/result-view.js be suitable for calling the service?

I don't think so. That method is just manipulating the model state and has no access to the view. The best place to do it is in the constructor of ResultView after the call to etch.initialize. Then you'll be able to reach out and grab your icon ref with confidence that it exists. Do you need to call into the service more than once for a given result? I don't think you do but maybe I'm wrong.

Contributor

nathansobo commented Nov 10, 2017

Would the didAddResult method in lib/project/result-view.js be suitable for calling the service?

I don't think so. That method is just manipulating the model state and has no access to the view. The best place to do it is in the constructor of ResultView after the call to etch.initialize. Then you'll be able to reach out and grab your icon ref with confidence that it exists. Do you need to call into the service more than once for a given result? I don't think you do but maybe I'm wrong.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

Do you need to call into the service more than once for a given result? I don't think you do but maybe I'm wrong.

The old, path-based service did, and that was why I made my changes in the render method. But the element-icons service only needs to be called once per DOM element. Everything else is taken care of from there...

Contributor

Alhadis commented Nov 10, 2017

Do you need to call into the service more than once for a given result? I don't think you do but maybe I'm wrong.

The old, path-based service did, and that was why I made my changes in the render method. But the element-icons service only needs to be called once per DOM element. Everything else is taken care of from there...

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

If you need to call back into the service after every update, you can do it in the writeAfterUpdate hook. Assuming the service may want to mutate the DOM node this is the best place to put it to ensure the update is grouped with any other DOM writes so as to prevent layout thrashing.

Contributor

nathansobo commented Nov 10, 2017

If you need to call back into the service after every update, you can do it in the writeAfterUpdate hook. Assuming the service may want to mutate the DOM node this is the best place to put it to ensure the update is grouped with any other DOM writes so as to prevent layout thrashing.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

The services is conservative about performing DOM changes, don't worry. :)

Come to think about it, there's no need for the legacy icon-service to be called multiple times, either. It isn't like a search result's path will change and warrant a different icon...

I'm now using this:

    etch.initialize(this);
    getIconServices().updateIcon(this);
  }

With the getIconClasses method of IconServices replaced with the much more logical (and consistent) updateIcon method instead:

updateIcon (view) {
  if (this.elementIcons) {
    this.elementIconDisposables.add(this.elementIcons(view.refs.icon, view.filePath))
  } else {
    let iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
    if (Array.isArray(iconClass)) {
      iconClass = iconClass.join(' ')
    }
    view.refs.icon.classList.add(...iconClass.trim().split(/\s+/))
  }
}

Weirdly, not all of the icons are visible when the search results pane is first opened:

Figure 1

Note the missing icon at the bottom.

Contributor

Alhadis commented Nov 10, 2017

The services is conservative about performing DOM changes, don't worry. :)

Come to think about it, there's no need for the legacy icon-service to be called multiple times, either. It isn't like a search result's path will change and warrant a different icon...

I'm now using this:

    etch.initialize(this);
    getIconServices().updateIcon(this);
  }

With the getIconClasses method of IconServices replaced with the much more logical (and consistent) updateIcon method instead:

updateIcon (view) {
  if (this.elementIcons) {
    this.elementIconDisposables.add(this.elementIcons(view.refs.icon, view.filePath))
  } else {
    let iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
    if (Array.isArray(iconClass)) {
      iconClass = iconClass.join(' ')
    }
    view.refs.icon.classList.add(...iconClass.trim().split(/\s+/))
  }
}

Weirdly, not all of the icons are visible when the search results pane is first opened:

Figure 1

Note the missing icon at the bottom.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

That is weird. This new code structure looks better to me though. Do you need to call the disposable when the view is disposed though? Meaning do you want to say something like this.iconDisposable = getIconServices().updateIcon(this) and then dispose it in destroy?

Contributor

nathansobo commented Nov 10, 2017

That is weird. This new code structure looks better to me though. Do you need to call the disposable when the view is disposed though? Meaning do you want to say something like this.iconDisposable = getIconServices().updateIcon(this) and then dispose it in destroy?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

Should this have really gone somewhere in the ResultView.update method instead of the constructor?

Contributor

Alhadis commented Nov 10, 2017

Should this have really gone somewhere in the ResultView.update method instead of the constructor?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

Ah right, I forgot about the destroy part...

Contributor

Alhadis commented Nov 10, 2017

Ah right, I forgot about the destroy part...

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 10, 2017

Contributor

Well, update is only called when the result view needs to re-render. The initial render is actually performed via etch.initialize in the constructor. May need to call it in both places depending on how we're calling update and what is changing when we call it. If it can blow away the icon DOM element then it may warrant another call.

Contributor

nathansobo commented Nov 10, 2017

Well, update is only called when the result view needs to re-render. The initial render is actually performed via etch.initialize in the constructor. May need to call it in both places depending on how we're calling update and what is changing when we call it. If it can blow away the icon DOM element then it may warrant another call.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 10, 2017

Contributor

I'm still not sure what I could be doing wrong, then. :(
https://github.com/Cutlery-Drawer/find-and-replace/blob/icon-service/lib/project/result-view.js

What do I need to call? The update() method in the constructor, like this?

  etch.initialize(this);
  getIconServices().updateIcon(this);
  this.update();
}

EDIT: Okay, no, this.update(); didn't work, and neither did etch.update(this);. I'm stuck as to what to do here.... and I'm puzzled why the render function is off-limits. 😕

Contributor

Alhadis commented Nov 10, 2017

I'm still not sure what I could be doing wrong, then. :(
https://github.com/Cutlery-Drawer/find-and-replace/blob/icon-service/lib/project/result-view.js

What do I need to call? The update() method in the constructor, like this?

  etch.initialize(this);
  getIconServices().updateIcon(this);
  this.update();
}

EDIT: Okay, no, this.update(); didn't work, and neither did etch.update(this);. I'm stuck as to what to do here.... and I'm puzzled why the render function is off-limits. 😕

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 11, 2017

Contributor

@nathansobo I have no idea what to do. :) The closest solution I can see is what I attempted doing in #971, with the this.waitToAttach approach. I can simply store the iconDisposable on a WeakMap, rather than the Etch view...

Contributor

Alhadis commented Nov 11, 2017

@nathansobo I have no idea what to do. :) The closest solution I can see is what I attempted doing in #971, with the this.waitToAttach approach. I can simply store the iconDisposable on a WeakMap, rather than the Etch view...

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 13, 2017

Contributor

@Alhadis, sorry, busy weekend. Do you have a branch I could pull down and a way to reproduce the issues you're seeing so I can help?

Contributor

nathansobo commented Nov 13, 2017

@Alhadis, sorry, busy weekend. Do you have a branch I could pull down and a way to reproduce the issues you're seeing so I can help?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 14, 2017

Contributor

Hey @nathansobo,

I'm afraid not. Things were becoming very messy locally so I ended up deleting everything I had and working off #971 instead. See #974.

Contributor

Alhadis commented Nov 14, 2017

Hey @nathansobo,

I'm afraid not. Things were becoming very messy locally so I ended up deleting everything I had and working off #971 instead. See #974.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment