Fixes T11532. The language selection for pastes is now a typeahead that is backed by pygments.dropdown-choices. There is still a bit of weirdness around making "auto-detection" the default state. To actually select a different language, you first need to remove the "auto detect" option that is pre-populated in a new paste. Other than that, it works as intended.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T11532: Replace paste language selection dropdown with a typeahead
- Commits
- rP9422596ebf2f: Converted Paste language selection to a typeahead
Create a new paste with a file extension that can be auto-detected.
Created a new paste and manually selected the language
Edited a paste and changed the language.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
src/applications/paste/typeahead/PasteLanguageSelectDatasource.php | ||
---|---|---|
42 | Having this be an option doesn't make as much sense here as it does in a drop-down. I think the ideal functionality would be for it to attempt to auto-detect if the language field is left blank rather than having an explicit value for auto-detection. |
Yeah, I agree. Get rid of that option and let's treat "no value" as "detect automatically".
I think there's a small chance this will be a bit confusing (users may feel compelled to select the right value and be unable to find it, since there isn't as much of a hint that we'll figure it out for them) but we could try adding a caption, or changing the placeholder text, or changing the label to "Force Highlight As", or removing the control from the "Create" form and moving it to the "Edit" form only, or probably some other things if there's confusion.
Code changes look correct to me.
This currently works in every case for pre-existing pastes (highlights correctly, and doesn't show a language on the edit page if no language was selected).
However, if I try to save a paste with no language selected right now I get this exception:
So I'm currently trying to track down where that is happening so I can modify it to handle empty values for pastes.
If you haven't already, I think you can set phabricator.developer-mode to get stack traces. Otherwise, the trace should be in your webserver error log or in DarkConsole, usually.
- Added migration to allow language to be nullable
We are no longer requiring the user to select a language. If they don't select a language, we'll attempt to guess the language based on the file extension.
Two minor SQL consistency things:
Although bin/storage adjust will fix it anyway, it's slightly better to include the collation explicitly:
ALTER TABLE {$NAMESPACE}_pastebin.pastebin_paste MODIFY language VARCHAR(64) COLLATE {$COLLATE_TEXT};
With the collation included, MySQL will only need to do one table copy to apply the mutation (from non-nullable straight to nullable with the correct collation).
Without the collation, MySQL may (in some configurations) need to do two: this one might make the column nullable but leave it with the wrong (default) collation, and then bin/storage adjust would fix the collation later.
The net effect is the same, but if there are a ton of pastes the migration might be a lot slower if it has to make two stops along the journey.
Second, you should name this migration something.01.something and add a something.02.something (that is, make sure your second migration is alphabetically later than the first one) which makes the data consistent for existing pastes:
UPDATE paste SET language = NULL WHERE language = '';
Then old and new pastes should look the same data-wise, and save us from any weird stuff down the line like "the API says '' for some of them and null for others".
src/applications/paste/editor/PhabricatorPasteEditEngine.php | ||
---|---|---|
92 | With the null migration, I think you should be able to remove this conditional. |
Oh weird. Now when I go to create a new Paste I get this:
That's the value it shows when it gets set to empty string rather than null. Still digging in to where that empty string is coming from.
(The rest of this looks good to me now.)
src/applications/paste/typeahead/PasteLanguageSelectDatasource.php | ||
---|---|---|
12 | Oh, nice. |
Thanks! I did find one bug while kicking the tires on this but it's not related to your changes so I'll file it separately.