Same as the other one - Githubissues

mg979 commented 6 years ago

I don't know where to merge it, here would seem better to me but it says that it can't be done automatically. If you can do it here, the other one can be closed. Sorry, I'm not very practical with these things.

oleg-shilo commented 6 years ago

OK. Let's pause and try to merge everything. I am also not very fluent in resolving conflicts but at least they are simple in our case.

I will resolve conflicts and let you know the outcome.

mg979 commented 6 years ago

Unless there are problems, I won't add anything else for now. If you can't solve the conflicts, you can create another branch from master and merge it there, then delete mg979-dev. Your previous changes(with unimportant additions) are stored anyway in the commit 'Integrated changes'

mg979 commented 6 years ago

There is one more important fix (just one replaced line) but I'll add it when you're ready

oleg-shilo commented 6 years ago

I don't feel good about it :)

We have 3 active PRs. Two of them are for dev (non master) branch. One of the later two has conflicts. I am still receiving notification on same commits.

To my surprise the GitHub command line instructions do not work as expected. Never happens to me with GitHub before.

It's becoming very messy. Sorry but we have to truly stop and try to make some sense of it. Have to go now. Will resume tonight.

As for your feedback on my changes to your PR9 I am happy to commit/merge.

Your suggestion for on_pre_close is very correct one. I just completely forgot about it. Tried to use it and discovered that it has its own issues. IE it's fired twice on closing the tab and every time it's a different grouping context. Decided to keep the inelegant but simple approach I have already tested.

I noticed the the changes you have done with key mappings. All good except I would prefer not to change the already existing (and know by users) default mapping for for visibility toggling (Alt+M, Alt+M). Thus I have modified your mapping a bit:

    { "keys": ["alt+m", "alt+m"],    "command": "show_code_map" },
    { "keys": ["alt+m", "alt+n"],    "command": "navigate_code_map", "args": {"start": true} },
    { "keys": ["alt+m", "alt+."],    "command": "synch_code_map" },
    { "keys": ["alt+m", "alt+,"],    "command": "synch_code_map" },

(Alt+M, Alt+M) - toggle map visibility (Alt+M, Alt+N) - map navigate mode (Alt+M, Alt+.) - synch to right (map) or left (source) depending on the focused view. Note '.' is a '>' button) (Alt+M, Alt+,) - synch to right (map) or left (source) depending on the focused view. Note ',' is a '<' button)

Though I will not change it in your branches but do it after merge.

mg979 commented 6 years ago

Ok do it as you feel more comfortable with, definitely I made too many changes in a short time, sorry for that.

mg979 commented 6 years ago

If you think it's better, just merge your changes in your dev branch, then I'll clone it and proceed from there, and trash the rest.

oleg-shilo commented 6 years ago

OK, we are getting there. :) I have fully merged the all changes to the dev

Now I will need to commit a few changes to fix conflict resolving mistakes I made and the branch will be ready for the further development (or ->master merge).

I will do the testing and let you know if anything needs to be addressed. This time I will try to stay away from changing the code in parallel with you so the merges will be easier.

mg979 commented 6 years ago

Ok sorry if I made some mess.. too much stuff.. in my now separated branch I got it working, waiting for your version this time. My dev right now is in synch with yours, and I'll keep it like that.

oleg-shilo commented 6 years ago

Don't worry. This stuff happens and me being not entirely fluent with git branch management also doesn't help.

The dev branch is now ready so you can sync. And the testing report is coming your way (in ~10 mins).

oleg-shilo commented 6 years ago

OK, I have done the testing. Found a few things that need to be addressed but nothing major. Can you please have a look at them. I am not sure what is the best way for us to communicate about the matter as this PR is closed and I only have a naked the dev branch a s a start point. Let me know what you think. And, kind of obvious, let's do the changes against that dev branch.

The error message for the unsupported source views has two user experience problems:
- It has hard codded hotkey, which may be invalid if the user has customized the mapping.
- Engaging the hot key as message suggests does not change anything as the view is still unsupported. May be having a generic message (e.g. "Unsupported document type") or even an empty string is the way to go?
I appreciated your new "sync from view" use-case. It's is mapped now nicely to Alt+M, Alt+> buttons. It's not an issue but just a comment. :)
When CodeMap is placed in the group that already has a view the map view tab is not made active. In result the map view is empty until user activates the tab and clicks on the source. This is a difficult one as ST3 focus management is a pain in the neck.
Another nasty focus management problem. Can only be reproduced with toggling visibility via shortcut. Have empty document having focus and map view group having extra view. Toggle map view visibility 2-3 rounds. Eventually it will loose the focus and activate the different document in the main view group:

code_map_2

Honestly, I believe it's the same cause and once #3 fixed this one will also be gone.

mg979 commented 6 years ago

ok first here is my version based on your commit, with some changes. I won't commit it for now:

https://gist.github.com/mg979/3a360443cd2142946470d28a4c560f07

You can overwrite your file, make a diff. I suggest you use GitSavvy for this, and make "inline diff", it's easier to see changes in this case.

Then:

-1. it is supported but it needs the changes(1 extra arg) in the gist; the message can be changed, the hotkey shouldn't be a problem more than the others, it can be remapped. In the future I could integrate it in synch_code_map command so that it uses its hotkey but I'm not sure it's a good idea.

-3. I don't have this problem with my gist version above, but maybe I don't follow the same steps. In my version a new group is always added, they aren't joined.

-4. I think here the problem is that the plugin is not designed to have another tab in the same group, this can surely be fixed but I should look into it, probably needs some extra checks.

mg979 commented 6 years ago

I made a fake PR so that you can see what's changed

https://github.com/oleg-shilo/sublime-codemap/pull/14

oleg-shilo / sublime-codemap

Same as the other one #12