oleg-shilo / sublime-codemap

CodeMap - is a ST3 plugin for showing the code tree representing the code structure of the active view/document
MIT License
41 stars 4 forks source link

New PR, lot of work #9

Closed mg979 closed 6 years ago

mg979 commented 6 years ago

I'd prefer that you had a dev branch so you could test it. I've tested it but I can't be sure it's all perfect. If you could make a dev branch so that I can merge it there and you can give me your opinions on things, I would prefer.

General fixes:

New features:

Of these, the universal mapper needs to be expanded, and the navigable map could use some refinements, For now one has to be careful to exit the navigation with left/right/escape, or the function can get stuck. I'll correct it, but I'd like some feedback too. I hope you'll like it and that you'll have time to take a look.

Ah, I forgot: I had to restructure a lot of things. Many functions have been moved to a 'support' script, so that they don't stand in the way with the things they have little in common with. Other functions have been isolated so I could use them modularly. Then some changes here and there for better functioning (I hope no bugs). Please activate trim_automatic_white_space in Sublime Text preferences too. Bye

oleg-shilo commented 6 years ago

Started working on your PR.

I am very happy with what you have done for CodeMap navigation.

Particularly appreciate the explicit "navigation mode". I tried to play focus back to document on selection of the map node and it works but for the complicated scenarios when one needs to place focus on a specific vied, do some stuff and move focus back the solution was not reliable. Yes, focus management in ST3 leaves much to be desired. Thus your move of be more explicit about focused view operations is a correct one.

Mapping depth works very well. 👍

I have a few small problems that I will provide you feedback on later. But the overall impression is very positive.

Thank you.

mg979 commented 6 years ago

Thanks for the appreciation, I am aware that it's still not perfect. If you notice something strange or that you don't like or you'd do differently, please tell me, these days I'll work on it another bit trying to fix some some things.

oleg-shilo commented 6 years ago

Please be aware that I have already committed some changes in your branch. Will try to finalize them today.

mg979 commented 6 years ago

Ok, the changes I made in the last commit were very small (the commit seems big but the real changes are a few lines).

oleg-shilo commented 6 years ago

OK, all done. I have committed the changes that address those a few small problems I mentioned. Please process my feedback and let me know if you agree with the code modifications or further changes are required.

  1. Very minor. The _universalmapper produces "Decoding failed" when incompatible document (e.g. *.cs) is selected. Correct me if it is caused by other reasons. Arguably it's better to have the mapper output as empty string instead. Semantics of "Decoding failed" implies failure or an error condition while it is in fact an expected valid condition.

  2. Closing map_view should trigger closing (if enabled in settings) of the view's group if the group has no other views. It only worked for toggling the map_view visibility but not for the view closed by the mouse click. I have fixed this one. Though it was tricky as when on_close is fired the view is already disassociated from the group thus it's impossible to evaluate if the group contains any other views. Had to cache map_view parent group,

  3. This one is more conceptual. The universal mapper returns the mapping result, while any standard mapper expected to return a "generate" routine that is to be invoked when mapping needs to be refreshed. The idea was that when one needs to test if the document can be mapped the mapper will return very quickly the "generator" routine without generating the map. And costly generation will only be performed when map is to be displayed. With the universal mapper the map is generated twice on the document activation. Once from code_map_generator.can_map(file) and second time from code_map_generator.run(self, edit, **args). I changed the run to handle the simple and universal mappers differently.

    if using_universal_mapper:
       (map, syntax) = code_map_generator.get_mapper(source)
        map_syntax = syntax
    else:
        (generate, syntax) = code_map_generator.get_mapper(source)
        map = generate(source)
        map_syntax = syntax

    I am OK with the change even if it isn't elegant at all. Thus if universal_mapper causes performance issues (I haven't seen any) then it will need to follow the simple mapper model.

I would rather merge this PR as soon as practical so the branches discrepancy does not grow.

mg979 commented 6 years ago

Thanks, now I'm going to read them.

Very minor. The universal_mapper produces "Decoding failed"...

Ok, I just wrote there for fun, while testing things.

Closing map_view should trigger closing (if enabled in settings) of the view's group if the group has no other views...

I never use the mouse to close views so I never noticed it, but I read somewhere that the API treats operations made with tab buttons differently than in other cases, maybe on_pre_close() would be useful in that case but I don't know. Still if your changes work it's ok.

With the universal mapper the map is generated twice on the document activation.

I realized this today when I was working on changes that I didn't commit because I only wanted to commit a quick fix. I also changed this aspect, probably in a different way. What I did in my uncommitted changes is to store the generated map first, then reuse it if valid, instead of rerunning the mapper generation for the same file. I will see how you did it, maybe it's better than what I did. In my uncommitted changes I can make a map from views that don't have a file(like extracted zipped files, pasted code in unsaved buffers, etc) so I wouldn't like to lose that thing.

mg979 commented 6 years ago

closing this