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

Support Windows Terminal #183

Merged
merged 5 commits into from
May 1, 2020
Merged

Support Windows Terminal #183

merged 5 commits into from
May 1, 2020

Conversation

@sylveon
Copy link
Collaborator

sylveon commented Apr 29, 2020

Nice work!

I'm getting the following error when trying to use the adapter however.

PS C:\Users\Hyper-V> pokemon
Traceback (most recent call last):
  File "C:\Users\Hyper-V\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\Scripts\pokemon-script.py", line 11, in <module>
    load_entry_point('pokemon-terminal==1.2.0', 'console_scripts', 'pokemon')()
  File "C:\Users\Hyper-V\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\site-packages\pokemonterminal\main.py", line 113, in main
    scripter.change_terminal(target.get_path())
  File "C:\Users\Hyper-V\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\site-packages\pokemonterminal\scripter.py", line 81, in change_terminal
    TERMINAL_PROVIDER.change_terminal(image_file_path)
  File "C:\Users\Hyper-V\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\site-packages\pokemonterminal\terminal\adapters\windowsterminal.py", line 50, in change_terminal
    WindowsTerminalProvider.set_background_image(path)
  File "C:\Users\Hyper-V\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\site-packages\pokemonterminal\terminal\adapters\windowsterminal.py", line 20, in set_background_image
    if (profile['guid'] == current_profile):
TypeError: string indices must be integers

Seems like it's forgetting that it should access data['profiles']['list']. Wouldn't it make more sense for it to change the defaults profile however?

Like this

# update defaults profile
profile = data['profiles']['defaults']
if (path is None):
    del profile['backgroundImage']
else:
    profile['backgroundImage'] = path

@kyle-seongwoo-jun
Copy link
Contributor Author

kyle-seongwoo-jun commented Apr 30, 2020

@sylveon Thank you!

Seems like it's forgetting that it should access data['profiles']['list'].

Oh, I didn't know there's Default Profile Settings option.

I'll change to set profiles to data['profiles']['list'] when data['profiles'] is dictionary type,

Wouldn't it make more sense for it to change the defaults profile however?

I'm not sure... but I think it seems to be better to change just the current profile only.

I think there are some users who doesn't want to change another profile.

@sylveon
Copy link
Collaborator

sylveon commented Apr 30, 2020

Changing only the current profile conflicts with the existing behavior of the app when it comes to terminal apps which don't support per-console background customization, like Tilix.

@kyle-seongwoo-jun
Copy link
Contributor Author

Okay, I got it.

@kyle-seongwoo-jun
Copy link
Contributor Author

@sylveon I updated it. Please check it. Thank you 😄

Copy link
Collaborator

@sylveon sylveon left a comment

Choose a reason for hiding this comment

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

Nice work! Props for adding a migration, didn't even think of that.

@sylveon sylveon merged commit 3ef5930 into LazoCoder:master May 1, 2020
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.

2 participants