Skip to content

Conversation

@janepie
Copy link
Member

@janepie janepie commented May 14, 2025

  • adds a setting to toggle tools
  • checks for availability of activated tools (if the relevant app is not installed, it's not imported)

image

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice! Feedback/questions:

  • We generally use "Sentence case", so "Context agent", "Public transport", "Office document generation" etc
  • We try to avoid extra scrolling containers inside the main one. Could we just show all of them, or maybe grouping helps, or having 2 columns, left for active and right for inactive settings?

@janepie
Copy link
Member Author

janepie commented May 14, 2025

Nice! Feedback/questions:

* [ ]  We generally use "Sentence case", so "Context agent", "Public transport", "Office document generation" etc

will do

* [ ]  We try to avoid extra scrolling containers inside the main one. Could we just show all of them, or maybe grouping helps, or having 2 columns, left for active and right for inactive settings?

These are declarative settings in AppAPI, the design is not defined here. Maybe @andrey18106 can help here?

@janepie
Copy link
Member Author

janepie commented May 14, 2025

image

@julien-nc
Copy link
Member

About the scrolling, maybe this can be changed to individual values instead of one value that combines all tools. This way each checkbox is independent and the generic rendering won't put them all in a scrollable area.
Wdyt @janepie ?

@julien-nc
Copy link
Member

Also 👍 !

@marcelklehr
Copy link
Member

We generally use "Sentence case"

"Context agent" is a proper name, though. Wondering if that still applies in this case (?)

@janepie
Copy link
Member Author

janepie commented May 14, 2025

About the scrolling, maybe this can be changed to individual values instead of one value that combines all tools. This way each checkbox is independent and the generic rendering won't put them all in a scrollable area. Wdyt @janepie ?

This takes up a lot more space as every entry needs a title line, and we create more db querys. I would prefer to keep it in one value, if we do not want it scrollable in general it's maybe best to change the UI of SettingsFieldType.MULTI_CHECKBOX?

@janepie janepie force-pushed the feat/tool-toggling branch from 3b40ef6 to 0c7da0b Compare May 14, 2025 15:01
@marcelklehr
Copy link
Member

Yay, the integration test is already catching stuff 🎉

2025-05-14 15:11:09,256 - context_agent - ERROR - Error: Traceback (most recent call last):
  File "/home/runner/work/context_agent/context_agent/context_agent/ex_app/lib/main.py", line 135, in background_thread_task
    output = react(task, nextcloud)
  File "/home/runner/work/context_agent/context_agent/context_agent/ex_app/lib/agent.py", line 49, in react
    safe_tools, dangerous_tools = get_tools(nc)
  File "/home/runner/work/context_agent/context_agent/context_agent/ex_app/lib/tools.py", line 19, in get_tools
    is_activated = json.loads(nc.appconfig_ex.get_value('tool_status'))
  File "/opt/hostedtoolcache/Python/3.10.17/x64/lib/python3.10/json/__init__.py", line 339, in loads
    raise TypeError(f'the JSON object must be str, bytes or bytearray, '
TypeError: the JSON object must be str, bytes or bytearray, not NoneType

@marcelklehr
Copy link
Member

I really like the refactor we did 🎉

@marcelklehr marcelklehr merged commit a264ec8 into main May 20, 2025
4 checks passed
@marcelklehr marcelklehr deleted the feat/tool-toggling branch May 20, 2025 12:54
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.

5 participants