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

Formatting improvements #14

Merged
merged 8 commits into from
Aug 28, 2017
Merged

Formatting improvements #14

merged 8 commits into from
Aug 28, 2017

Conversation

eugenesvk
Copy link
Contributor

As agreed, sending a PR to slightly improve the format of your extension

NB! please do check that these changes don't break the indicator on selection (it's not working for me for some reason even with a fresh extension install, so maybe it's just the changes to fman core, but maybe I've made a mistake somewhere in the code)

@kek91
Copy link
Owner

kek91 commented Aug 17, 2017

Thank you for this! I'll do some testing before I merge it in regarding the selection indicator you mentioned. I'm currently swamped so might have to delay it just a couple of days.

@kek91
Copy link
Owner

kek91 commented Aug 23, 2017

Sorry for my delay here!

The reason selection doesn't work is because the justification variables (justFd, justFl, justSz) only exists as local properties in the refresh() method. You can fix it by adding them as class-wide properties or simply adding them in the show_selected_files() method as well.

Would you like to fix it?

@eugenesvk
Copy link
Contributor Author

No worries, adding the variables in the other method seems simple enough, so I'll take a stab at fixing it and update the PR

@eugenesvk
Copy link
Contributor Author

Hey, I've moved the variables to a 'Just' class as you suggested and made a reference to a class in both show_selected_files() and refresh() methods, but I still can't test it — as mentioned before, even with a freshly installed fman version (your original addon, no themes), selection doesn't work for me for some reason, though I can see the regular status bar just fine.

@kek91 kek91 merged commit a2bd17e into kek91:master Aug 28, 2017
@kek91
Copy link
Owner

kek91 commented Aug 28, 2017

Awesome, thank you :)
FYI it only works when selecting 1 and 1 file using Spacebar. "Select all" and "Deselect" doesn't trigger StatusBarExtended

@kek91 kek91 mentioned this pull request Aug 28, 2017
@eugenesvk
Copy link
Contributor Author

eugenesvk commented Aug 28, 2017

Ah, got it, – it only works when toggle_selection command has been explicitly called as a command, rather than as an argument, but I've mapped all my keys (e.g. space) to have that passed as an argument (i.e. command is move_cursor_down with "args" toggle_selection)
Got it working now via a dedicated key, which doesn't move cursor, just selects/delesects.
Is that something that fman has to fix API-wise or is there a way to actually listen to those toggle_selection as well?

@kek91
Copy link
Owner

kek91 commented Aug 29, 2017

I'm not sure, I had to overwrite the toggle_selection method from the Core plugin. I guess we have to do the same for select_all and deselect as well. Haven't looked much at it yet, hopefully soon.
Edit: Feel free to take a stab at it if you'd like :)

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.

None yet

2 participants