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

Add new feature to find file(recreate) #9889

Merged
merged 1 commit into from Jan 7, 2016
Merged

Add new feature to find file(recreate) #9889

merged 1 commit into from Jan 7, 2016

Conversation

@koreamic
Copy link
Contributor

@koreamic koreamic commented Dec 5, 2015

Using the fuzzy lib, develop "file finder" feature.
(http://feedback.gitlab.com/forums/176466-general/suggestions/4987909-add-file-finder-fuzzy-input-in-files-tab-to-ju)

Issue(#9854)
PR(#9855)

(ps. cause by my mistake, recreate PR(#9855), sorry!!)

@stanhu
Copy link
Member

@stanhu stanhu commented Dec 9, 2015

Great work! I assume since this depends on this gitlab_git MR, this PR has to wait until a new version is release?

Can you add tests for this?

include ExtractsPath
include ActionView::Helpers::SanitizeHelper
include TreeHelper
before_action :assign_ref_vars

This comment has been minimized.

@stanhu

stanhu Dec 12, 2015
Member

Do we also need to add:

  before_action :require_non_empty_project
  before_action :authorize_download_code!
@stanhu
Copy link
Member

@stanhu stanhu commented Dec 12, 2015

Does this PR also need an upgrade of gitlab_git to 7.2.22?

@stanhu
Copy link
Member

@stanhu stanhu commented Dec 12, 2015

Also, this feature could use a spinach test to test the JavaScript functionality.

@koreamic
Copy link
Contributor Author

@koreamic koreamic commented Dec 12, 2015

@stanhu
Thank you for your concern and help.

Yes. This PR needs an upgrade of gitlab_git to 7.2.22.

and okay I will check to use a spinach test.

If you need to additional works. please comment.

Thank you. have a nice day.

@@ -14,6 +14,7 @@ class @ShortcutsNavigation extends Shortcuts
Mousetrap.bind('g m', -> ShortcutsNavigation.findAndFollowLink('.shortcuts-merge_requests'))
Mousetrap.bind('g w', -> ShortcutsNavigation.findAndFollowLink('.shortcuts-wiki'))
Mousetrap.bind('g s', -> ShortcutsNavigation.findAndFollowLink('.shortcuts-snippets'))
Mousetrap.bind('t', -> ShortcutsNavigation.findAndFollowLink('.shortcuts-find-file'))

This comment has been minimized.

This comment has been minimized.

@koreamic

koreamic Dec 15, 2015
Author Contributor

Okay, @Razer6 Thank you for your suggestions.
I updated _shortcuts.html.haml and screenshot.
and I added shortcuts using up and down array key in find file view to select file more conveniently.

The reason that add t shortcut in app/assets/javascripts/shortcuts_navigation.coffee is to use find file view on other page(eg. merge request/commit/branch page) If necessary.

but If not necessary, I just define it on the tree view.

what do you think about that?

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

I think it doesn't make sense on the issues page, but it does on any file pages (tree, blob, blame) as well as the project root.

@koreamic
Copy link
Contributor Author

@koreamic koreamic commented Dec 15, 2015

@stanhu
I added spinach test case and shortcuts that up down arrow to select item easly.
and modified some bugs(main menu bar out focusing)

please check and review sources

thank you.

@yms9654
Copy link
Contributor

@yms9654 yms9654 commented Dec 17, 2015

👍 This is a great feature!
I think many users like this.

@@ -0,0 +1,1158 @@
(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){

This comment has been minimized.

@dzaporozhets

dzaporozhets Dec 18, 2015
Member

Please move this file to /vendor/assets/javascripts

@dzaporozhets
Copy link
Member

@dzaporozhets dzaporozhets commented Dec 18, 2015

Please provide a screenshots for this feature


# make tbody row html
makeHtml: (filePath, blobItemUrl) ->
return @row.replace("%blobItemUrl", blobItemUrl).replace("%filePath", filePath)

This comment has been minimized.

@stanhu

stanhu Dec 19, 2015
Member

What happens if the file path has the text %blobItemUrl? Can we just construct the entire text without a replace?

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

As mentioned above, please generate the row DOM using jQuery.

# go back page
goBack: (event) ->
if event.keyCode == 27
event.preventdefault

This comment has been minimized.

@stanhu

stanhu Dec 19, 2015
Member

Isn't this event.preventDefault()?

return false;

# select row and focus
selecteRow: (rows, seletedRow, which) ->

This comment has been minimized.

@stanhu

stanhu Dec 19, 2015
Member

Is this a misspelling: selectRow or selectedRow?

$(document).on "keyup", @element, (event) =>
keyCode = event.keyCode if event
# up down right left arrow(37~40) esc(27) caps(20) ctrl(17) alt(18) shift(16) enter(13) cmd(91)
if (keyCode < 37 || keyCode > 40) && keyCode != 20 && keyCode != 17 && keyCode != 18 && keyCode != 16 && keyCode != 13 && keyCode != 91

This comment has been minimized.

@stanhu

stanhu Dec 19, 2015
Member

Should we define these key codes as constants (e.g. https://github.com/cybertoothca/keyevent/blob/master/src/keyevent.js)?

constructor: (@options)->
@filePaths = {}
@element = ".file-finder-input"
@row = "<tr class='tree-item'><td class='tree-item-file-name'><i class='fa fa-file-text-o fa-fw'></i><span class='str-truncated'><a href=%blobItemUrl>%filePath</a></span></td></tr>"

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Please build this structure using jQuery. Doing it as a string opens us up to XSS.

# init key event
initKeyEvent: ->
# unbind and bind keyup/keydown event at text input box
$(document).off "keyup"

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

I don't think we want to get rid of all keyup events. Can you namespace these to be specific to ProjectFindFile?

From https://api.jquery.com/bind/:

If the eventType string contains a period (.) character, then the event is namespaced. The period character separates the event from its namespace. For example, in the call .bind( "click.name", handler ), the string click is the event type, and the string name is the namespace. Namespacing allows us to unbind or trigger some events of a type without affecting others. See the discussion of .unbind() for more information.

# $(document).on "keyup", @goBack
$(document).on "keyup", (event) =>
if event.keyCode == 27
event.preventdefault

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

event.preventDefault()

@renderList result, searchTxt

# select row and focus
selecteRow: (rows, seletedRow, which) ->

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

  • selectRow
  • selectedRow
  • which -> keyCode

# 38 up 40 down
if keyCode == 38 || keyCode == 40
@selecteRow $("tr.tree-item"), $('.tree-item.selected'), keyCode

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Inconsistent indentation


# find file
findFile: (searchTxt) ->
result = if searchTxt then fuzzaldrinPlus.filter(@filePaths, searchTxt) else @filePaths

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Inconsistent indentation

$(document).on "keyup", @element, (event) =>
keyCode = event.keyCode if event
# up down right left arrow(37~40) esc(27) caps(20) ctrl(17) alt(18) shift(16) enter(13) cmd(91)
if (keyCode < 37 || keyCode > 40) && keyCode != 20 && keyCode != 17 && keyCode != 18 && keyCode != 16 && keyCode != 13 && keyCode != 91

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Why are we checking all these keycodes?

This comment has been minimized.

@Razer6

Razer6 Dec 22, 2015
Member

We already use moustrap (https://github.com/ccampbell/mousetrap), is this keycode checking necessary with that?

# 38 up 40 down
if keyCode == 38 || keyCode == 40
@selecteRow $("tr.tree-item"), $('.tree-item.selected'), keyCode
return false

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

No need to return since there are no statements after this if/else.

method: "get"
dataType: "json"
success: $.proxy((data) ->
$(".loading").hide()

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

We need to make this more specific in case we get other .loadings in the future.

url: url
method: "get"
dataType: "json"
success: $.proxy((data) ->

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Why do we need $.proxy?

$(".loading").hide()
@filePaths = data
@findFile($(@element).val())
@selecteRow $("tr.tree-item")

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

We need to make all of these more specific :) Can we pass a wrapper element to new ProjectFindFile, and then use @wrapper.find('tr.tree-item') etc?

renderList: (filePaths, searchTxt) ->
$(".tree-table > tbody").empty()
for filePath, i in filePaths
break if i > 20

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

This means we break when i == 21, which means we had 0..20 before this: 21 renders :) We need only 20.

selecteRow: (rows, seletedRow, which) ->
if rows && rows.length > 0
if seletedRow && seletedRow.length > 0
next = null

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

Why is this necessary?

$(".tree-table > tbody").append(html)

# highlight text(awefwbwgtc -> <b>a</b>wefw<b>b</b>wgt<b>c</b> )
highlightText = (txt, charArr) ->

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

This is the highlighting code that the Atom editor uses: https://github.com/atom/fuzzy-finder/blob/master/lib%2Ffuzzy-finder-view.coffee#L70-L88 Maybe we can use the same logic :)


@url = namespace_project_files_path(@project.namespace, @project, @ref, @options.merge(format: :json))
@treeUrl = namespace_project_tree_path(@project.namespace, @project, @ref)
@blobUrlTemplate = namespace_project_blob_path(project.namespace, project, tree_join(@id || @commit.id))

This comment has been minimized.

@DouweM

DouweM Dec 22, 2015
Member

We don't need all of these in the controller, we can just use the helpers in the view.


# render result
renderList: (filePaths, searchTxt) ->
$(".tree-table > tbody").empty()

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

Please find this within @element or something.

$(".tree-table > tbody").empty()

for filePath, i in filePaths
break if i > 19

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

So if i == 20? :) That may be a little clearer.

break if i > 19
highlightFilePath = filePath

if searchTxt

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

searchTxt.length > 0


blobItemUrl = "#{@options.blobUrlTemplate}/#{filePath}"
html = @makeHtml highlightFilePath, blobItemUrl
$(".tree-table > tbody").append(html)

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

Please use @element.find or similar.


# make tbody row html
makeHtml: (filePath, blobItemUrl) ->
return $("<tr class='tree-item'><td class='tree-item-file-name'><i class='fa fa-file-text-o fa-fw'></i><span class='str-truncated'><a href=#{blobItemUrl}>#{filePath}</a></span></td></tr>")

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

To prevent XSS when blobItemUrl or filePath contain something that looks like HTML/JS, Build the span element like you do now, and then create the link like $('<a>').attr('href', blobItemUrl).text(filePath) and add it to the span using jQuery.

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

Also, no need to explicitly return in CoffeeScript :)

location.href = @options.treeUrl

goToBlob: =>
path = $('.tree-item.selected .tree-item-file-name a').attr('href')

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

@element.find please!


goToBlob: =>
path = $('.tree-item.selected .tree-item-file-name a').attr('href')
if path then location.href = path

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

location.href = path if path is more CoffeeScript-like.

Mousetrap.stopCallback = (event, element, combo) =>
if combo == 'up' or combo == 'down' or combo == 'esc' or combo == 'enter'
return element != @projectFindFile.inputElement[0]
return _oldStopCallback(event, element, combo)

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

Why do we need to override Mousetrap.stopCallback?

This comment has been minimized.

@koreamic

koreamic Jan 7, 2016
Author Contributor

@DouweM
When focus in textbox, Mousetrap's shortcuts is not working.
I think that we want to fire shortcuts action at that time in find file page.
so I overrided Mousetrap.stopCallback
and added event.preventDefault for avoiding to move cusor in textbox.
(when up/down key pressed, cusor is moved to home/end)

= render 'projects/last_push'

- if can? current_user, :download_code, @project
.tree-download-holder
= render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true
= render 'projects/find_file_link'

This comment has been minimized.

@DouweM

DouweM Jan 5, 2016
Member

We need a little bit of distance between the "Find File" and "Download zip" buttons. This works:

  • Change _find_file_link.html to:
= link_to namespace_project_find_file_path(@project.namespace, @project, @ref), class: 'btn btn-grouped shortcuts-find-file', rel: 'nofollow' do
  = icon('search')
  %span Find File
  • Change in tree/show.html.haml from:
- if can? current_user, :download_code, @project
  .tree-download-holder
    = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true
= render 'projects/find_file_link'

to:

.pull-right
  = render 'projects/find_file_link'
  - if can? current_user, :download_code, @project
    = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'hidden-xs hidden-sm btn-grouped', split_button: true
@DouweM
Copy link
Member

@DouweM DouweM commented Jan 5, 2016

@koreamic I may seem very critical (and I am), but I tried it out locally and it works really, really well. Great job. I can't wait to have this in the next release :) I'm sure we can get it ready.

@koreamic
Copy link
Contributor Author

@koreamic koreamic commented Jan 7, 2016

@DouweM
I have to thank you for your review and concern all the time.
I have learned many things.
I modified sources and commited as your review.
but If there are things to be modified, leave comments.

Using the fuzzy filter, develop "file finder" feature.
- feedback(http://feedback.gitlab.com/forums/176466-general/suggestions/4987909-add-file-finder-fuzzy-input-in-files-tab-to-ju )
- fuzzy filter(https://github.com/jeancroy/fuzzaldrin-plus)
- shortcuts(when "t" was hitted at tree view, go to 'file find' page and 'esc' is to go back)
- depends on gitlab_git 7.2.22
@DouweM
Copy link
Member

@DouweM DouweM commented Jan 7, 2016

@koreamic You're amazing. I've got nothing, I'm merging this.

DouweM added a commit that referenced this pull request Jan 7, 2016
Add new feature to find file(recreate)
@DouweM DouweM merged commit 5c7de85 into gitlabhq:master Jan 7, 2016
1 check passed
1 check passed
semaphoreci The build passed on Semaphore.
Details
@JobV
Copy link
Contributor

@JobV JobV commented Jan 7, 2016

This is awesome. Thanks @koreamic

@DouweM
Copy link
Member

@DouweM DouweM commented Jan 7, 2016

@koreamic I notice you're not credited in the changelog! I added (koreamic) in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2327, let me know if you want that to have your full name instead.

@Razer6
Copy link
Member

@Razer6 Razer6 commented Jan 7, 2016

❤️

@stanhu
Copy link
Member

@stanhu stanhu commented Jan 7, 2016

Great work, @koreamic!

@koreamic
Copy link
Contributor Author

@koreamic koreamic commented Jan 7, 2016

@DouweM @stanhu @Razer6 @randx @JobV @yms9654 @noveler17
Thanks a lot for your help & patience.
@DouweM koreamic is enough. thanks

@dzaporozhets
Copy link
Member

@dzaporozhets dzaporozhets commented Jan 7, 2016

@koreamic can you please provide us with your full name? We prefer not to use usernames since it can be different between websites and can produce unnecessary confusion. cc @DouweM

@dzaporozhets
Copy link
Member

@dzaporozhets dzaporozhets commented Jan 7, 2016

@koreamic and thank you for getting this feature done! 👍

@koreamic
Copy link
Contributor Author

@koreamic koreamic commented Jan 8, 2016

@randx @DouweM
Okay. My full name is Kyungchul Shin
Thank you.

@dzaporozhets
Copy link
Member

@dzaporozhets dzaporozhets commented Jan 8, 2016

@koreamic thank you!

@DouweM I will update CHANGELOG

@dzaporozhets
Copy link
Member

@dzaporozhets dzaporozhets commented Jan 8, 2016

wow @stanhu already updated CHANGELOG 😄 🚀

@rspeicher
Copy link
Member

@rspeicher rspeicher commented Jan 17, 2016

@koreamic Curious, why did you use fuzzaldrin-plus instead of fuzzaldrin? And which version was it?

@DouweM
Copy link
Member

@DouweM DouweM commented Jan 17, 2016

@tsigo That was at my suggestion actually. I had much better results in Atom's fuzzy finder with the "Use Alternate Scoring" option enabled, which switches from fuzzaldrin to fuzzaldrin-plus. It's now the default in Atom's fuzzy finder as well: atom/fuzzy-finder@c004c7e

@rspeicher
Copy link
Member

@rspeicher rspeicher commented Jan 17, 2016

Ok. Do we know which version it was? I'm trying to replace the minified vendor JS with non-minified versions (specifically so we know version info, among other things).

@DouweM
Copy link
Member

@DouweM DouweM commented Jan 17, 2016

@tsigo Ah, good call. It doesn't really matter, the API hasn't changed, so you can just get the latest one.

stanhu pushed a commit to gitlabhq/gitlab_git that referenced this pull request Feb 9, 2016
Added function ls_files in Gitlab::Git::Repository

Using native git command(git ls-tree)
ls_files function ls to return only each file's full paths of a repository

This function is called by Repository within GitLab as @DouweM 's suggestion.

(ref : gitlabhq/gitlabhq#9889 & https://github.com/gitlabhq/gitlabhq#9855)

See merge request !60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants