From 795e624d5df52789f08d4e2d82e9a07e9dfd5cc7 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 28 Sep 2017 15:31:31 +0200 Subject: [PATCH] Add ability to specify alternative text for media attachments (#5123) * Fix #117 - Add ability to specify alternative text for media attachments - POST /api/v1/media accepts `description` straight away - PUT /api/v1/media/:id to update `description` (only for unattached ones) - Serialized as `name` of Document object in ActivityPub - Uploads form adjusted for better performance and description input * Add tests * Change undo button blend mode to difference --- app/controllers/api/v1/media_controller.rb | 10 +- app/javascript/mastodon/actions/compose.js | 38 ++++ .../components/extended_video_player.js | 14 +- .../mastodon/components/media_gallery.js | 3 +- .../mastodon/components/video_player.js | 204 ------------------ .../features/compose/components/upload.js | 96 +++++++++ .../compose/components/upload_form.js | 44 +--- .../compose/containers/upload_container.js | 21 ++ .../containers/upload_form_container.js | 13 +- .../features/ui/components/media_modal.js | 5 +- .../features/ui/components/video_modal.js | 1 + .../features/ui/util/async-components.js | 4 - .../mastodon/features/video/index.js | 4 +- app/javascript/mastodon/reducers/compose.js | 19 +- app/javascript/styles/components.scss | 47 +++- app/lib/activitypub/activity/create.rb | 2 +- app/models/media_attachment.rb | 7 + .../activitypub/note_serializer.rb | 6 +- .../rest/media_attachment_serializer.rb | 3 +- config/routes.rb | 2 +- ...09_add_description_to_media_attachments.rb | 5 + db/schema.rb | 3 +- .../api/v1/media_controller_spec.rb | 29 +++ spec/models/media_attachment_spec.rb | 9 +- 24 files changed, 311 insertions(+), 278 deletions(-) delete mode 100644 app/javascript/mastodon/components/video_player.js create mode 100644 app/javascript/mastodon/features/compose/components/upload.js create mode 100644 app/javascript/mastodon/features/compose/containers/upload_container.js create mode 100644 db/migrate/20170927215609_add_description_to_media_attachments.rb diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index 8a1992fca4..9f330f0dfe 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -10,7 +10,7 @@ class Api::V1::MediaController < Api::BaseController respond_to :json def create - @media = current_account.media_attachments.create!(file: media_params[:file]) + @media = current_account.media_attachments.create!(media_params) render json: @media, serializer: REST::MediaAttachmentSerializer rescue Paperclip::Errors::NotIdentifiedByImageMagickError render json: file_type_error, status: 422 @@ -18,10 +18,16 @@ class Api::V1::MediaController < Api::BaseController render json: processing_error, status: 500 end + def update + @media = current_account.media_attachments.where(status_id: nil).find(params[:id]) + @media.update!(media_params) + render json: @media, serializer: REST::MediaAttachmentSerializer + end + private def media_params - params.permit(:file) + params.permit(:file, :description) end def file_type_error diff --git a/app/javascript/mastodon/actions/compose.js b/app/javascript/mastodon/actions/compose.js index 9f10a8c150..8be5b939f9 100644 --- a/app/javascript/mastodon/actions/compose.js +++ b/app/javascript/mastodon/actions/compose.js @@ -37,6 +37,10 @@ export const COMPOSE_COMPOSING_CHANGE = 'COMPOSE_COMPOSING_CHANGE'; export const COMPOSE_EMOJI_INSERT = 'COMPOSE_EMOJI_INSERT'; +export const COMPOSE_UPLOAD_CHANGE_REQUEST = 'COMPOSE_UPLOAD_UPDATE_REQUEST'; +export const COMPOSE_UPLOAD_CHANGE_SUCCESS = 'COMPOSE_UPLOAD_UPDATE_SUCCESS'; +export const COMPOSE_UPLOAD_CHANGE_FAIL = 'COMPOSE_UPLOAD_UPDATE_FAIL'; + export function changeCompose(text) { return { type: COMPOSE_CHANGE, @@ -165,6 +169,40 @@ export function uploadCompose(files) { }; }; +export function changeUploadCompose(id, description) { + return (dispatch, getState) => { + dispatch(changeUploadComposeRequest()); + + api(getState).put(`/api/v1/media/${id}`, { description }).then(response => { + dispatch(changeUploadComposeSuccess(response.data)); + }).catch(error => { + dispatch(changeUploadComposeFail(id, error)); + }); + }; +}; + +export function changeUploadComposeRequest() { + return { + type: COMPOSE_UPLOAD_CHANGE_REQUEST, + skipLoading: true, + }; +}; +export function changeUploadComposeSuccess(media) { + return { + type: COMPOSE_UPLOAD_CHANGE_SUCCESS, + media: media, + skipLoading: true, + }; +}; + +export function changeUploadComposeFail(error) { + return { + type: COMPOSE_UPLOAD_CHANGE_FAIL, + error: error, + skipLoading: true, + }; +}; + export function uploadComposeRequest() { return { type: COMPOSE_UPLOAD_REQUEST, diff --git a/app/javascript/mastodon/components/extended_video_player.js b/app/javascript/mastodon/components/extended_video_player.js index 5ab5e9e586..f8bd067e8e 100644 --- a/app/javascript/mastodon/components/extended_video_player.js +++ b/app/javascript/mastodon/components/extended_video_player.js @@ -5,6 +5,7 @@ export default class ExtendedVideoPlayer extends React.PureComponent { static propTypes = { src: PropTypes.string.isRequired, + alt: PropTypes.string, width: PropTypes.number, height: PropTypes.number, time: PropTypes.number, @@ -31,15 +32,20 @@ export default class ExtendedVideoPlayer extends React.PureComponent { } render () { + const { src, muted, controls, alt } = this.props; + return (
); diff --git a/app/javascript/mastodon/components/media_gallery.js b/app/javascript/mastodon/components/media_gallery.js index a81409871e..38b26b1fc7 100644 --- a/app/javascript/mastodon/components/media_gallery.js +++ b/app/javascript/mastodon/components/media_gallery.js @@ -136,7 +136,7 @@ class Item extends React.PureComponent { onClick={this.handleClick} target='_blank' > - + {attachment.get('description')} ); } else if (attachment.get('type') === 'gifv') { @@ -146,6 +146,7 @@ class Item extends React.PureComponent {
diff --git a/app/javascript/mastodon/features/ui/util/async-components.js b/app/javascript/mastodon/features/ui/util/async-components.js index b8c5e885a3..ad5493f8c5 100644 --- a/app/javascript/mastodon/features/ui/util/async-components.js +++ b/app/javascript/mastodon/features/ui/util/async-components.js @@ -90,10 +90,6 @@ export function MediaGallery () { return import(/* webpackChunkName: "status/media_gallery" */'../../../components/media_gallery'); } -export function VideoPlayer () { - return import(/* webpackChunkName: "status/video_player" */'../../../components/video_player'); -} - export function Video () { return import(/* webpackChunkName: "features/video" */'../../video'); } diff --git a/app/javascript/mastodon/features/video/index.js b/app/javascript/mastodon/features/video/index.js index f228e434b3..069264ef53 100644 --- a/app/javascript/mastodon/features/video/index.js +++ b/app/javascript/mastodon/features/video/index.js @@ -104,6 +104,7 @@ export default class Video extends React.PureComponent { static propTypes = { preview: PropTypes.string, src: PropTypes.string.isRequired, + alt: PropTypes.string, width: PropTypes.number, height: PropTypes.number, sensitive: PropTypes.bool, @@ -247,7 +248,7 @@ export default class Video extends React.PureComponent { } render () { - const { preview, src, width, height, startTime, onOpenVideo, onCloseVideo, intl } = this.props; + const { preview, src, width, height, startTime, onOpenVideo, onCloseVideo, intl, alt } = this.props; const { progress, dragging, paused, fullscreen, hovered, muted, revealed } = this.state; return ( @@ -260,6 +261,7 @@ export default class Video extends React.PureComponent { loop role='button' tabIndex='0' + aria-label={alt} width={width} height={height} onClick={this.togglePlay} diff --git a/app/javascript/mastodon/reducers/compose.js b/app/javascript/mastodon/reducers/compose.js index 9d39584fc8..082d4d3705 100644 --- a/app/javascript/mastodon/reducers/compose.js +++ b/app/javascript/mastodon/reducers/compose.js @@ -22,6 +22,9 @@ import { COMPOSE_VISIBILITY_CHANGE, COMPOSE_COMPOSING_CHANGE, COMPOSE_EMOJI_INSERT, + COMPOSE_UPLOAD_CHANGE_REQUEST, + COMPOSE_UPLOAD_CHANGE_SUCCESS, + COMPOSE_UPLOAD_CHANGE_FAIL, } from '../actions/compose'; import { TIMELINE_DELETE } from '../actions/timelines'; import { STORE_HYDRATE } from '../actions/store'; @@ -220,15 +223,15 @@ export default function compose(state = initialState, action) { map.set('idempotencyKey', uuid()); }); case COMPOSE_SUBMIT_REQUEST: + case COMPOSE_UPLOAD_CHANGE_REQUEST: return state.set('is_submitting', true); case COMPOSE_SUBMIT_SUCCESS: return clearAll(state); case COMPOSE_SUBMIT_FAIL: + case COMPOSE_UPLOAD_CHANGE_FAIL: return state.set('is_submitting', false); case COMPOSE_UPLOAD_REQUEST: - return state.withMutations(map => { - map.set('is_uploading', true); - }); + return state.set('is_uploading', true); case COMPOSE_UPLOAD_SUCCESS: return appendMedia(state, fromJS(action.media)); case COMPOSE_UPLOAD_FAIL: @@ -256,6 +259,16 @@ export default function compose(state = initialState, action) { } case COMPOSE_EMOJI_INSERT: return insertEmoji(state, action.position, action.emoji); + case COMPOSE_UPLOAD_CHANGE_SUCCESS: + return state + .set('is_submitting', false) + .update('media_attachments', list => list.map(item => { + if (item.get('id') === action.media.id) { + return item.set('description', action.media.description); + } + + return item; + })); default: return state; } diff --git a/app/javascript/styles/components.scss b/app/javascript/styles/components.scss index da479347b5..631cd7a134 100644 --- a/app/javascript/styles/components.scss +++ b/app/javascript/styles/components.scss @@ -335,12 +335,52 @@ .compose-form__uploads-wrapper { display: flex; + flex-direction: row; padding: 5px; + flex-wrap: wrap; } .compose-form__upload { flex: 1 1 0; + min-width: 40%; margin: 5px; + + &-description { + position: absolute; + z-index: 2; + bottom: 0; + left: 0; + right: 0; + box-sizing: border-box; + background: linear-gradient(0deg, rgba($base-shadow-color, 0.8) 0, rgba($base-shadow-color, 0.35) 80%, transparent); + padding: 10px; + opacity: 0; + transition: opacity .1s ease; + + input { + background: transparent; + color: $ui-secondary-color; + border: 0; + padding: 0; + margin: 0; + width: 100%; + font-family: inherit; + font-size: 14px; + font-weight: 500; + + &:focus { + color: $white; + } + } + + &.active { + opacity: 1; + } + } + + .icon-button { + mix-blend-mode: difference; + } } .compose-form__upload-thumbnail { @@ -352,13 +392,6 @@ width: 100%; } -.compose-form__upload-cancel { - background-size: cover; - border-radius: 4px; - height: 100px; - width: 100px; -} - .compose-form__label { display: block; line-height: 24px; diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 4e19b30964..55addd66ec 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -105,7 +105,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity next if unsupported_media_type?(attachment['mediaType']) || attachment['url'].blank? href = Addressable::URI.parse(attachment['url']).normalize.to_s - media_attachment = MediaAttachment.create(status: status, account: status.account, remote_url: href) + media_attachment = MediaAttachment.create(status: status, account: status.account, remote_url: href, description: attachment['name'].presence) next if skip_download? diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index e4a974f968..25e41c209a 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -16,6 +16,7 @@ # shortcode :string # type :integer default("image"), not null # file_meta :json +# description :text # require 'mime/types' @@ -58,6 +59,7 @@ class MediaAttachment < ApplicationRecord validates_attachment_size :file, less_than: 8.megabytes validates :account, presence: true + validates :description, length: { maximum: 140 }, if: :local? scope :attached, -> { where.not(status_id: nil) } scope :unattached, -> { where(status_id: nil) } @@ -78,6 +80,7 @@ class MediaAttachment < ApplicationRecord shortcode end + before_create :prepare_description, unless: :local? before_create :set_shortcode before_post_process :set_type_and_extension before_save :set_meta @@ -136,6 +139,10 @@ class MediaAttachment < ApplicationRecord end end + def prepare_description + self.description = description.strip[0...140] unless description.nil? + end + def set_type_and_extension self.type = VIDEO_MIME_TYPES.include?(file_content_type) ? :video : :image extension = appropriate_extension diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index f94c3b9dc0..4dbf6a4444 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -89,12 +89,16 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer class MediaAttachmentSerializer < ActiveModel::Serializer include RoutingHelper - attributes :type, :media_type, :url + attributes :type, :media_type, :url, :name def type 'Document' end + def name + object.description + end + def media_type object.file_content_type end diff --git a/app/serializers/rest/media_attachment_serializer.rb b/app/serializers/rest/media_attachment_serializer.rb index f6e7c79d1f..e6e9c8e822 100644 --- a/app/serializers/rest/media_attachment_serializer.rb +++ b/app/serializers/rest/media_attachment_serializer.rb @@ -4,7 +4,8 @@ class REST::MediaAttachmentSerializer < ActiveModel::Serializer include RoutingHelper attributes :id, :type, :url, :preview_url, - :remote_url, :text_url, :meta + :remote_url, :text_url, :meta, + :description def id object.id.to_s diff --git a/config/routes.rb b/config/routes.rb index cb7e84d7bd..ad2d8fca23 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -193,7 +193,7 @@ Rails.application.routes.draw do get '/search', to: 'search#index', as: :search resources :follows, only: [:create] - resources :media, only: [:create] + resources :media, only: [:create, :update] resources :apps, only: [:create] resources :blocks, only: [:index] resources :mutes, only: [:index] diff --git a/db/migrate/20170927215609_add_description_to_media_attachments.rb b/db/migrate/20170927215609_add_description_to_media_attachments.rb new file mode 100644 index 0000000000..db8d765664 --- /dev/null +++ b/db/migrate/20170927215609_add_description_to_media_attachments.rb @@ -0,0 +1,5 @@ +class AddDescriptionToMediaAttachments < ActiveRecord::Migration[5.1] + def change + add_column :media_attachments, :description, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index e16599d323..90f8a56833 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170924022025) do +ActiveRecord::Schema.define(version: 20170927215609) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -161,6 +161,7 @@ ActiveRecord::Schema.define(version: 20170924022025) do t.string "shortcode" t.integer "type", default: 0, null: false t.json "file_meta" + t.text "description" t.index ["account_id"], name: "index_media_attachments_on_account_id" t.index ["shortcode"], name: "index_media_attachments_on_shortcode", unique: true t.index ["status_id"], name: "index_media_attachments_on_status_id" diff --git a/spec/controllers/api/v1/media_controller_spec.rb b/spec/controllers/api/v1/media_controller_spec.rb index baa22d7e48..0e494638f2 100644 --- a/spec/controllers/api/v1/media_controller_spec.rb +++ b/spec/controllers/api/v1/media_controller_spec.rb @@ -101,4 +101,33 @@ RSpec.describe Api::V1::MediaController, type: :controller do end end end + + describe 'PUT #update' do + context 'when somebody else\'s' do + let(:media) { Fabricate(:media_attachment, status: nil) } + + it 'returns http not found' do + put :update, params: { id: media.id, description: 'Lorem ipsum!!!' } + expect(response).to have_http_status(:not_found) + end + end + + context 'when not attached to a status' do + let(:media) { Fabricate(:media_attachment, status: nil, account: user.account) } + + it 'updates the description' do + put :update, params: { id: media.id, description: 'Lorem ipsum!!!' } + expect(media.reload.description).to eq 'Lorem ipsum!!!' + end + end + + context 'when attached to a status' do + let(:media) { Fabricate(:media_attachment, status: Fabricate(:status), account: user.account) } + + it 'returns http not found' do + put :update, params: { id: media.id, description: 'Lorem ipsum!!!' } + expect(response).to have_http_status(:not_found) + end + end + end end diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index f6717b7d5e..f20698c450 100644 --- a/spec/models/media_attachment_spec.rb +++ b/spec/models/media_attachment_spec.rb @@ -17,7 +17,6 @@ RSpec.describe MediaAttachment, type: :model do expect(media.file.meta["original"]["height"]).to eq 128 expect(media.file.meta["original"]["aspect"]).to eq 1.0 end - end describe 'non-animated gif non-conversion' do @@ -50,4 +49,12 @@ RSpec.describe MediaAttachment, type: :model do expect(media.file.meta["small"]["aspect"]).to eq 400.0/267 end end + + describe 'descriptions for remote attachments' do + it 'are cut off at 140 characters' do + media = Fabricate(:media_attachment, description: 'foo' * 100, remote_url: 'http://example.com/blah.jpg') + + expect(media.description.size).to be <= 140 + end + end end