-
Notifications
You must be signed in to change notification settings - Fork 23
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
follow up for the ticket https://github.com/bmag/emacs-purpose/issues/106 #107
base: master
Are you sure you want to change the base?
Conversation
…t for imenu for sh mode, added also one more tab to purpose-code-layout (useful for python shells, searching over the project, etc)
And yes, I'll cleanup this pull request to mimic current main project state once we solve issue with layout which doesn't work properly being in separated file. Sorry.. for some wrong changes here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I added comments on some technicalities.
-
Please don't change code1, instead write a code2 based on code1.
-
I was expecting to see changes to
purpose-x-code1-purpose-config
as well. (aspurpose-x-code2-purpose-config
).
window-purpose-x.el
Outdated
@@ -125,14 +137,15 @@ If a non-buffer-dedicated window with purpose 'dired exists, display | |||
the directory of the current buffer in that window, using `dired'. | |||
If there is no window available, do nothing. | |||
If current buffer doesn't have a filename, do nothing." | |||
(when (and (buffer-file-name) | |||
(when (not (string= (purpose--buffer-major-mode (current-buffer)) "org-mode")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(eq major-mode 'org-mode)
instead of (string= (purpose--...) "org-mode")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unless ...)
instead of (when (not ...))
window-purpose-x.el
Outdated
@@ -158,7 +171,7 @@ imenu." | |||
(purpose-set-extension-configuration :purpose-x-code1 purpose-x-code1-purpose-config) | |||
(purpose-x-code1--setup-ibuffer) | |||
(purpose-x-code1-update-dired) | |||
(imenu-list-minor-mode) | |||
(ignore-errors (imenu-list-minor-mode)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ignore-errors
necessary?
window-purpose-x.el
Outdated
@@ -168,7 +181,7 @@ imenu." | |||
(interactive) | |||
(purpose-del-extension-configuration :purpose-x-code1) | |||
(purpose-x-code1--unset-ibuffer) | |||
(imenu-list-minor-mode -1) | |||
(ignore-errors (imenu-list-minor-mode -1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ignore-errors
necessary?
Thanks, I reviewed the PR and submitted feedback. As I suggested in the issue report, please implement the change as a new extension, and don't change code1 itself. Copy code1 and rename code1 to code2, then do the changes you want to code2. If I'm not clear feel free to ask questions. What exactly is "the issue with layout which doesn't work properly being in separated file"? |
Hi bmag! |
Oh yes, sorry forgot to ask you: what do you mean by "add code2" instead of "changing code1" ? |
Hey bmag!
That works fine now. And I added my layout into your folder "layouts" |
Yes, I'm talking about adding separate variables and functions. Right now, these exist: (defvar purpose-x-code1--window-layout ...)
(defvar purpose-x-code1-purpose-config ...)
(defvar purpose-x-code1-buffers-changed ...)
(define-ibuffer-filter purpose-x-code1-ibuffer-files-only ...)
(defun purpose-x-code1--setup-ibuffer ...)
(defun purpose-x-code1--unset-ibuffer ...)
(defun purpose-x-code1-update-dired ...)
(defun purpose-x-code1-update-changed ...)
(defun purpose-x-code1-setup ...)
(defun purpose-x-code1-unset ...) What I'm saying is to add below them: (defvar purpose-x-code2--window-layout ...)
(defvar purpose-x-code2-purpose-config ...)
(defun purpose-x-code2-update-dired ...)
(defun purpose-x-code2-update-changed ...)
(defun purpose-x-code2-setup ...)
(defun purpose-x-code2-unset ...) I think you can reuse
I prefer the layout to be stored in
I prefer the necessary configuration to be stored in
It's weird that you need to wrap |
Ok, now it's clear |
Any news? |
Hi, I scheduled it but last weeks very crazy. Will do that asap. Sorry :( for delay. |
1 similar comment
1 similar comment
Hello dear bmag. |
@sergeyglazyrindev It's ok, I understand, hope you're all better now and wish you the best. As for this PR, I'm very busy myself this month, so it will take me some time to review these. Worst case scenario, I will review them towards the end of February. |
Hey, sorry for the late response, I was really busy and turns out "end of February" wasn't the worst case I know putting together this PR is a considerable amount of work, and I appreciate the effort you put into it, but I feel it ended up too big and out-of-scope for Purpose itself, so I won't be merging it. However, I'd like to convert it to a new page in Purpose's wiki. Would you mind if I did that? |
Hey bmag!
I moved out almost all not necessary stuff from window-purpose-x.el though there's one problem:
maybe you can advice (I am debugging it now - maybe there's a problem with my init.el)
Hm, I removed everything from my init.el except window-purpose initialization... and still there's a problem: if I fix default layout in your window-purpose-x.el then it works, if I put the same layout to separated layout file it doesn't work... Any idea ?
I included changes to layout, so you may play with these changes.....