Page MenuHomePhabricator

Converted Paste language selection to a typeahead
ClosedPublic

Authored by jcox on Aug 26 2016, 9:08 PM.
Tags
None
Referenced Files
F14031894: D16463.id39621.diff
Sat, Nov 9, 12:11 PM
F14031893: D16463.id39618.diff
Sat, Nov 9, 12:11 PM
F14031892: D16463.id39617.diff
Sat, Nov 9, 12:11 PM
F14031891: D16463.id39615.diff
Sat, Nov 9, 12:10 PM
F14031890: D16463.id39610.diff
Sat, Nov 9, 12:10 PM
F14031889: D16463.id39597.diff
Sat, Nov 9, 12:10 PM
F14031888: D16463.id.diff
Sat, Nov 9, 12:10 PM
F14019711: D16463.diff
Tue, Nov 5, 11:30 PM

Details

Summary

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.

Test Plan

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
Branch
PasteLanguageTypeahead (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13490
Build 17349: Run Core Tests
Build 17348: arc lint + arc unit

Event Timeline

jcox retitled this revision from to Converted Paste language selection to a typeahead.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)
jcox edited edge metadata.
jcox added inline comments.
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.

epriestley added a reviewer: epriestley.

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 revision now requires changes to proceed.Aug 26 2016, 10:44 PM
jcox edited edge metadata.
  • Removed explicit auto-detect option

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:

pasted_file (150×525 px, 15 KB)

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.

jcox edited edge metadata.
  • 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.

epriestley edited edge metadata.

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
82

With the null migration, I think you should be able to remove this conditional.

This revision now requires changes to proceed.Aug 29 2016, 8:55 PM
jcox edited edge metadata.
  • Added data migration and removed un-needed conditional

Oh weird. Now when I go to create a new Paste I get this:

pasted_file (109×436 px, 6 KB)

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.

Might be PhabricatorPaste::initializeNewPaste().

(The rest of this looks good to me now.)

src/applications/paste/typeahead/PasteLanguageSelectDatasource.php
12

Oh, nice.

jcox edited edge metadata.
  • Don't set new paste language to ''
epriestley edited edge metadata.

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.

This revision is now accepted and ready to land.Aug 29 2016, 9:35 PM
This revision was automatically updated to reflect the committed changes.