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

"Invalid input: active browser columns not set" #1797

Closed
dae opened this issue Apr 13, 2022 · 7 comments
Closed

"Invalid input: active browser columns not set" #1797

dae opened this issue Apr 13, 2022 · 7 comments
Milestone

Comments

@dae
Copy link
Member

dae commented Apr 13, 2022

Can be triggered by opening the browse screen and leaving it open, then forcing a one-way sync.

Possible ways we could fix this:

  • close windows like the browse screen when we open and close the collection
  • provide a separate hook to perform actions on collection reopen
  • something else?
@dae dae added this to the 2.1.51 milestone Apr 13, 2022
@RumovZ
Copy link
Collaborator

RumovZ commented Apr 13, 2022

Maybe we could make this more robust in general by not requiring the frontend to set the columns? So instead of raising an invalid input error if active_browser_columns is None, the backend would deserialize it itself from the config.

@dae
Copy link
Member Author

dae commented Apr 14, 2022

The reason we did it that way is because the mobile clients want to use different config var, as the desktop config may have too many columns for small screens, and may include add-on-specific columns. Passing the relevant config key in to browser_row_for_id() could work instead, but it's a bit less efficient.

@RumovZ
Copy link
Collaborator

RumovZ commented Apr 14, 2022

Ah, I knew there must have been a reason. 🙂
And what if the backend knew on which client it is running? That is, it would get initiated with an enum variant Platform::Desktop or so, which later can be matched to retrieve the according config key. Do you think that would potentially be useful in other contexts as well?

@dae
Copy link
Member Author

dae commented Apr 14, 2022

It's a bit inflexible:

  • say someone decides to implement a TUI wrapper at one point - they wouldn't be able to have their own browser columns without submitting a patch to us to add that client to the enum
  • what if the desktop one day wanted to support multiple browser windows with different columns in each? (edit: though since we only store one state at the moment, that would require further work)

@RumovZ
Copy link
Collaborator

RumovZ commented Apr 14, 2022

I see. Also, on second thought, would it even be enough to reset the backend columns? Couldn't even the config values have changed after a reopen, which would require to also reset the frontend parts?

@dae
Copy link
Member Author

dae commented Apr 15, 2022

Yep, after a full download the keys may have changed, so it would require a re-read of the config vars. Closing the browse screen on a full sync is probably the easiest way to fix this :-)

@dae
Copy link
Member Author

dae commented Apr 19, 2022

I might do that for now as a band-aid, and we can consider alternative approaches in the future.

@dae dae closed this as completed in a7cb5e2 Apr 19, 2022
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

No branches or pull requests

2 participants