Skip to content
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

Show workspace name in doom-modeline #1049

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Show workspace name in doom-modeline #1049

merged 1 commit into from
Aug 16, 2023

Conversation

sebastiaanspeck
Copy link
Contributor

Fixes #1047 and #1048

@gmemstr
Copy link

gmemstr commented Aug 11, 2023

Testing locally, get the following. I think there's some more things that need catching @sebastiaanspeck

Debugger entered--Lisp error: (void-variable treemacs-segment)
  treemacs--setup-mode-line()
  #<subr treemacs-mode>()
  treemacs--mode-check-advice(#<subr treemacs-mode>)
  apply(treemacs--mode-check-advice #<subr treemacs-mode> nil)
  treemacs-mode()
  treemacs--init()
  treemacs(nil)
  funcall-interactively(treemacs nil)
  command-execute(treemacs)

@jtl5770
Copy link

jtl5770 commented Aug 11, 2023

I also tested it by patching my local treemacs - it behaves still the same (no output all) when using natively compiled.

@WouterSpekkink
Copy link

WouterSpekkink commented Aug 12, 2023

I also tested it by patching my local treemacs - it behaves still the same (no output all) when using natively compiled.

Same here.

EDIT: Actually, for some reason I ignored the last commit (sorry!) and with that commit it works on my machine.

@sebastiaanspeck
Copy link
Contributor Author

Ahh! I’ve also pushed a new workaround. Can some people test it?

@WouterSpekkink
Copy link

Ahh! I’ve also pushed a new workaround. Can some people test it?

#1050 solves it for me.

@gmemstr
Copy link

gmemstr commented Aug 12, 2023

Latest push produces

Debugger entered--Lisp error: (invalid-function doom-modeline-def-segment)
  doom-modeline-def-segment(nil "Display treemacs." #(" org " 0 5 (face (:inherit (mode-line-inactive doom-modeline-buffer-minor-mode)))))
  treemacs--setup-mode-line()
  #<subr treemacs-mode>()
  treemacs--mode-check-advice(#<subr treemacs-mode>)
  apply(treemacs--mode-check-advice #<subr treemacs-mode> nil)
  treemacs-mode()
  treemacs--init()
  treemacs(nil)
  funcall-interactively(treemacs nil)
  command-execute(treemacs)

The commit prior seems to have worked at first blush, but switching workspaces throws up the same error.

@sebastiaanspeck
Copy link
Contributor Author

Ahh! I’ve also pushed a new workaround. Can some people test it?

#1050 solves it for me.

Yep.. that's just the code how it was prior my proposed change to show the current workspace name in the modeline. Sad to see that my workarounds doesn't fix the issues. I've opened a issue on doom-modeline as the issue is purely related to the name you have to give to the segment and you can't use a string or symbol..

Maybe we need to merge #1050 for now and search for a good, clean and working solution together?

@sebastiaanspeck
Copy link
Contributor Author

Latest push produces


Debugger entered--Lisp error: (invalid-function doom-modeline-def-segment)

  doom-modeline-def-segment(nil "Display treemacs." #(" org " 0 5 (face (:inherit (mode-line-inactive doom-modeline-buffer-minor-mode)))))

  treemacs--setup-mode-line()

  #<subr treemacs-mode>()

  treemacs--mode-check-advice(#<subr treemacs-mode>)

  apply(treemacs--mode-check-advice #<subr treemacs-mode> nil)

  treemacs-mode()

  treemacs--init()

  treemacs(nil)

  funcall-interactively(treemacs nil)

  command-execute(treemacs)

The commit prior seems to have worked at first blush, but switching workspaces throws up the same error.

Sad to hear it only works for the first time ::/

@sebastiaanspeck sebastiaanspeck changed the title Fix for doom-modeline Draft: Fix for doom-modeline Aug 12, 2023
@sebastiaanspeck sebastiaanspeck marked this pull request as draft August 12, 2023 20:59
@BlindingDark
Copy link
Contributor

BlindingDark commented Aug 14, 2023

I am on treemacs 6a6171c, after I tried compiling treemacs first and then doom-modeline, then everything fine now.
To do this (I mean compile doom-modeline after treemacs), you can just delete the whole doom-modeline folder and let it re-download/re-compile.

edited:
Doesn't work now, seems to work only once, don't know why😭

edited 2:
try to use eval-and-compile, it works!

(with-no-warnings
  (eval-and-compile
    (doom-modeline-def-segment treemacs
    ...

Alexander-Miller pushed a commit that referenced this pull request Aug 14, 2023
Revert until #1049 has a working solution that also works with native compilation
@BlindingDark
Copy link
Contributor

BlindingDark commented Aug 15, 2023

After a few tries I found a solution (use eval-and-compile with require)

((fboundp 'doom-modeline)
 (eval-and-compile
   (require 'doom-modeline)
   (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
   (doom-modeline 'treemacs)))

It works without any warning in various cases. Please have a try!

@sebastiaanspeck sebastiaanspeck marked this pull request as ready for review August 15, 2023 05:03
@sebastiaanspeck
Copy link
Contributor Author

sebastiaanspeck commented Aug 15, 2023

This needs to be tested by others before merging another breaking change. It works on my machine :)

Needs to be tested without doom-modeline installed as well, since the GitHub Actions fail on line (require 'doom-modeline).

@sebastiaanspeck sebastiaanspeck changed the title Draft: Fix for doom-modeline Show workspace name in doom-modeline Aug 15, 2023
@seagle0128
Copy link

Should load doom-modeline package before calling doom-modeline-def-segment macro.
(require 'doom-modeline) or (with-eval-after-load 'doom-modeline) is necessary.
And, please be aware that doom-modeline-def-segment is a macro.

(doom-modeline 'treemacs))
((fboundp 'doom-modeline)
(eval-and-compile
(require 'doom-modeline)
Copy link

@seagle0128 seagle0128 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use with-eval-after-load instead of require since doom-modeline is not a dependency.

I think eval-and-compile is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

              ((and (fboundp 'doom-modeline)
		    (fboundp 'doom-modeline-def-segment)
		    (fboundp 'doom-modeline-face)
		    (fboundp 'doom-modeline-def-modeline))
	       (with-eval-after-load 'doom-modeline)
	       (doom-modeline-def-segment treemacs-workspace-name
		 "Display treemacs workspace name."
		 (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
			     'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
	       (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs-workspace-name))
	       (doom-modeline 'treemacs))

This triggers reference to free variable 'treemacs-workspace-name' again...

Copy link
Contributor

@BlindingDark BlindingDark Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((fboundp 'doom-modeline)
 (with-eval-after-load 'doom-modeline
   (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
   (doom-modeline 'treemacs))
 )

Got this error

Debugger entered--Lisp error: (void-variable treemacs)
  #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_8>()
  eval-after-load(doom-modeline #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_8>)
  treemacs--setup-mode-line()
  #f(compiled-function () (interactive nil) #<bytecode -0x1b2d643fdaa15b7>)()
  treemacs--mode-check-advice(#f(compiled-function () (interactive nil) #<bytecode -0x1b2d643fdaa15b7>))
  apply(treemacs--mode-check-advice #f(compiled-function () (interactive nil) #<bytecode -0x1b2d643fdaa15b7>) nil)
  treemacs-mode()

I think we need eval-and-compile and require because we need expand doom-modeline-def-segment macro.

Copy link

@seagle0128 seagle0128 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If doom-modeline is byte-compiled and loaded before treemacs, no such errors occur, I think.

@BlindingDark
Copy link
Contributor

Needs to be tested without doom-modeline installed as well, since the GitHub Actions fail on line (require 'doom-modeline).

Oh yes, I forgot this, and we should ignore warnings if doom-modeline is not installed.
So try this 🙏

((fboundp 'doom-modeline)
 (eval-and-compile
   (require 'doom-modeline nil 'noerror))
 (with-no-warnings
   (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs)))
 (doom-modeline 'treemacs))

@sebastiaanspeck
Copy link
Contributor Author

Latest commit works for me (and GitHub Actions), so please test and review 🙌

@seagle0128
Copy link

seagle0128 commented Aug 15, 2023

Try this:

((eval-and-compile (require 'doom-modeline nil 'noerror))
    (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
    (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
    (doom-modeline 'treemacs))

@sebastiaanspeck
Copy link
Contributor Author

Try this:

((require 'doom-modeline nil 'noerror)
  (eval-and-compile 
    (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
 (doom-modeline 'treemacs)))
image

@seagle0128
Copy link

Try this:

((require 'doom-modeline nil 'noerror)
  (eval-and-compile 
    (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
 (doom-modeline 'treemacs)))
image

should remove (fboundp 'doom-modeline)

@seagle0128
Copy link

seagle0128 commented Aug 15, 2023

Try this:

((eval-and-compile (require 'doom-modeline nil 'noerror))
    (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
   (doom-modeline 'treemacs))

I've updated the snippet. Try it.

@sebastiaanspeck
Copy link
Contributor Author

sebastiaanspeck commented Aug 15, 2023

Try this:

((eval-and-compile (require 'doom-modeline nil 'noerror))
    (doom-modeline-def-segment treemacs
     "Display treemacs."
     (propertize (format " %s " (treemacs-workspace->name (treemacs-current-workspace)))
                 'face (doom-modeline-face 'doom-modeline-buffer-minor-mode)))
   (doom-modeline-def-modeline 'treemacs '(bar " " major-mode) '(treemacs))
   (doom-modeline 'treemacs))

I've updated the snippet. Try it.

In toplevel form:
src/elisp/treemacs-mode.el:2[30](https://github.com/Alexander-Miller/treemacs/actions/runs/5865108952/job/15901395417#step:8:31):43: 
Error: reference to free variable ‘treemacs-workspace-name’

@gmemstr
Copy link

gmemstr commented Aug 15, 2023

I can confirm the current state the PR works for me!

image

@sebastiaanspeck
Copy link
Contributor Author

It does for me as well, but the Error: reference to free variable ‘treemacs-workspace-name’ is exactly the same error we had before, or is it fixed now using native compiled code?

@sebastiaanspeck
Copy link
Contributor Author

I can confirm the current state the PR works for me!

image

Would you be so kind to test the most recent commit? 🙏

@WouterSpekkink
Copy link

treemacs

Works for me as well. Switching workspaces also works fine (I thought I read in an earlier comment that it didn't before).

@gmemstr
Copy link

gmemstr commented Aug 15, 2023

Looks to be working fine for me :)

@sebastiaanspeck
Copy link
Contributor Author

Thank you for testing, let's launch the 🚀 and merge this!

@Alexander-Miller
Copy link
Owner

All right, let's do this 🤞

@Alexander-Miller Alexander-Miller merged commit 8be5913 into Alexander-Miller:master Aug 16, 2023
4 of 5 checks passed
@sebastiaanspeck sebastiaanspeck deleted the bugfix/fix-doom-modeline branch August 17, 2023 04:24
sebastiaanspeck added a commit to sebastiaanspeck/themes that referenced this pull request Mar 6, 2024
The current `doom-modeline` shows the current workspace name in `treemacs` since Alexander-Miller/treemacs#1049, thus making the `doom-modeline` useful again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Treemacs + Doom Modeline on emacs 29.1
7 participants