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

feat: support autoplay answer for tts model #1971

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

shaohuzhang1
Copy link
Contributor

feat: support autoplay answer for tts model

Copy link

f2c-ci-robot bot commented Jan 3, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

f2c-ci-robot bot commented Jan 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

<el-icon class="mr-4">
<Setting />
</el-icon>
</el-button>
</el-form-item>
</el-form>
<TTSModeParamSettingDialog ref="TTSModeParamSettingDialogRef" @refresh="refreshTTSForm" />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code has a couple of minor discrepancies that could be addressed:

  1. Typographical Errors:

    • The <el-checkbox> element is followed by a space in the HTML structure, which might cause display issues if it's not intentional. However, since there are no errors related to this line, we can ignore it.
  2. Duplicate Button:

    • There is an <el-button> with type="primary" after adding the <el-checkbox>. This button already exists within the same condition, so having another one might not make sense. If this is intended to show both "自动播放" (Auto Play) and the TTS parameter settings link simultaneously, you should ensure only one appears based on the state of form_data.tts_autoplay.
  3. Button Link:

    • Both buttons share the same @click event handler (openTTSParamSettingDialog). Since they perform the same action, it would be more efficient to use a single button that checks the current value of form_data.tts_autoplay or dynamically decides which action to take based on its state rather than repeating logic twice.

Here’s a refactored version considering these points:

<template>
  <!-- Existing code -->
</template>

<script lang="vue">
export default {
  // Previous component data, methods, etc.
};
</script>

Summary:

  • Ensure correct styling around the checkbox, especially if additional spacing is necessary.
  • Remove duplicate logic; prefer using conditional rendering instead of repeated elements when possible.
  • Adjust behavior as needed regarding the visibility or functionality of each button.

class="mr-8"
>
<el-icon class="mr-4"><Setting /></el-icon>
</el-button>
</el-form-item>
</el-form>
</el-scrollbar>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few potential issues and optimizations in the provided code:

  1. Duplicate Button: The button with @click="openTTSParamSettingDialog" is used twice under both conditions (v-if="applicationForm.tts_type === 'TTS'"). This might be unnecessary if you want to ensure it appears only when certain conditions are met.

    Optimization suggestion: Remove or combine one of these buttons based on your requirements.

  2. Unnecessary Checkbox for Playback Functionality: Adding an additional checkbox (<el-checkbox v-model="applicationForm.tts_autoplay">自动播放</el-checkbox>) for playback functionality seems redundant since the same state can be managed using the switch control (<el-switch>).

    Suggestion: Remove this checkbox and focus solely on managing the switch's state through <el-switch>.

  3. Conditional Placement: Ensure that the TTS-related elements (TTS autoplay settings and parameter setting button) are always visible under the TTS tab (v-if="activeTab !== 'audio' && applicationForm.tab === 'tts' || activeTab === 'edit-tts'"), rather than being conditionally visible based on specific form values and tabs.

  4. Accessibility Considerations: Double-check that all interactive components have appropriate ARIA attributes (aria-live, role) for better accessibility.

Here's a revised version of the relevant part of the code with some of these improvements considered:

<template>
  <div class="flex-between">
    <span class="mr-4">语音播放</span>
    <div>
      <!-- Remove or Combine the Duplicate Button -->
      
      <el-switch 
        size="small" 
        v-model="applicationForm.tts_model_enable"
        />
        
      <el-button 
        v-if="activeTab !== 'audio' && applicationForm.tab === 'tts' || activeTab === 'edit-tts'" 
        type="primary" 
        link 
        @click="openTTSParamSettingDialog"
        class="mr-8"
      >
        <el-icon class="mr-4"><Setting /></el-icon>
        设置
      </el-button>
    </div>
  </div>

  <el-select 
    v-if="...yourConditions..." 
    placeholder="选择模型...">
    ...
  </el-select>
  
  <!-- If needed, include other related controls here -->
</template>

<script>
export default {
  data() {
    return {
      // Your existing data here
      activeTab: '', // Example property to manage current active tab
      applicationForm: {
        tts_type: '', // etc.,
        tts_model_id: null,
        tts_model_enable: false
      }
    };
  },
  methods: {
    openTTSParamSettingDialog() {
      alert('Show dialog'); // Placeholder function logic
    }
  }
};
</script>

Ensure that any adjustments made align with your application's design guidelines and user experience needs.

name='tts_autoplay',
field=models.BooleanField(default=False, verbose_name='自动播放'),
),
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided migration script looks mostly correct but might benefit from some minor improvements:

  1. Docstring: It would be good to add comments explaining the purpose of each part of the migration script.

  2. Field Description: The verbose_name for tts_autoplay could include some context, although it already provides meaning with "自动播放".

  3. Empty Lines: There are extra empty lines that can be removed for better readability.

Here's an improved version of the code:

#+ Generated by Django 4.2.15 on 2025-01-03 14:07

+from django.db import migrations, models

#
# This migration adds a new boolean field 'tts_autoplay' to the Application model.
#

class Migration(migrations.Migration):

    dependencies = [
        ('application', '0021_applicationpublicaccessclient_client_id_and_more'),
    ]

    operations = [
        migrations.AddField(
            model_name='application',
            name='tts_autoplay',
            field=models.BooleanField(default=False, verbose_name='TTS 自动播放'),  # Include more descriptive text here if needed
        ),
    ]

Optimizations Suggestions:

  • Add Comments: Always include comments to explain what parts of the migration do. This makes it easier for others (or yourself in six months) to understand why changes were made.
  • Remove Empty Lines: These lines don’t contribute much value and clutter the code unnecessarily. Consider removing them to make it cleaner.
  • Field Descriptions: If possible, give your fields meaningful names that convey their purpose clearly. In this case, including additional context like "TTS" is good practice if the field specifically pertains to Text-to-Speech functionality.

>
<el-icon><Operation /></el-icon>
</el-button>
</div>
</el-form-item>
</el-form>
</el-scrollbar>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code looks mostly clean and well-organized. However, there are a few improvements that could be made:

  1. Consistent Code Structure: Ensure consistent indentation and spacing throughout the code for better readability.

  2. Code Duplication: The <el-switch> control is duplicated within two radio groups. It can be moved outside of both to avoid redundancy and improve cleanliness.

  3. Accessibility Checks: Consider adding aria labels or descriptions where necessary to ensure accessibility for keyboard users.

Here's an optimized version of the code with these suggestions applied:

@@ -402,96 +402,98 @@
     <template #label>
       <div class="flex-between">
         <span class="mr-4">语音播放</span>
+        <div class="flex">
          <el-checkbox v-model="applicationForm.tts_autoplay"></el-checkbox>
          
            <el-switch
              size="small"
              v-model="applicationForm.tts_model_enable"
              @change="ttsModelEnableChange"
            />
           
         </div>
         
       
         -
+

By making these changes, the code remains clear and concise while maintaining proper functionality and usability.

<el-icon><Operation /></el-icon>
</el-icon>
</el-button>
</div>
</el-form-item>
</el-form>
<TTSModeParamSettingDialog ref="TTSModeParamSettingDialogRef" @refresh="refreshTTSForm" />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a concise list of suggestions to improve the code:

Improvements

  1. Remove Redundant Code Blocks:

    • Remove one of the el-radio groups or the entire section if it is not needed.
  2. Optimize Logic in Radio Group:

    • The radio group can be simplified by removing the conditional rendering logic within each radio button.
  3. Simplify Select Options Rendering:

    • Remove redundancy in filtering options based on status and disable state, consolidate them into a single loop with inline conditions.
  4. Add Conditional Classes:

    • Use Vue's dynamic classes (class="{ active: condition }" instead of repeating similar class names.)
  5. Avoid Repeated HTML Tags:

    • Combine redundant <span>, <div>, and other tags when possible to simplify styling and structure.
  6. Consider Refactoring State Management:

    • If there are many related properties that change together, consider using computed properties or Vuex for better maintainability.
  7. Use Semantic Element Names Wisely:

    • While semantic elements like <div> may sometimes add clarity, avoid overusing them unless necessary. They should be used wisely according to their actual purpose (e.g., <section>, <article>).
  8. Update References Correctly:

    • Ensure all references (like @click) point to correctly named methods or components.

Example Revised Code

<template>
  <el-form-item label="语音播放" prop="tts_type">
    <el-flex direction="row-reverse">
      <span class="mr-4">自动播放:</span>
      <el-checkbox v-model="form_data.tts_autoplay"></el-checkbox>
      
      <el-switch 
        :value="form_data.tts_model_enable"
        @change="ttsModelEnableChange"
        style="vertical-align:middle;"
      >
        <!-- Tooltip could also be added here if needed -->
      </el-switch>
      
      
      <div class="flex-between w-full mt-4">
        <el-select  
          v-if="form_data.tts_type === 'TTS'"
          v-model="form_data.tts_model_id"
          class="w-full"
          @wheel="wheel"
          popper-class="select-model"
          @change="ttsModelChange()"

          placeholder="请选择语音合成模型">
          
          <el-option-group    
            v-for="(group, label) in ttsModelOptions"
            :key="label"
            :label="relatedObject(providerOptions, label, 'provider')?.name">

            <el-option   
              v-for="option in group.filter(item => item.status === 'SUCCESS')"
              :key="option.id"
              :label="option.name"
              :value="option.id"
              class="flex justify-between px-2 py-1 rounded border transition ease-in-out duration-200"
              :class="{ 'bg-green-500 text-white': option.id === form_data.tts_model_id }">

              <div>
                <img 
                  :src="relatedObject(providerOptions, label, 'provider')?.icon"
                  alt=""
                  height="20"
                  width="20"
                  class="inline-block mr-4 align-middle"
                />
                
                {{ option.name }}
                
                <span 
                  class="text-gray-500 ml-2 relative top-[0.5px]"
                  v-if="option.permission_type === 'PUBLIC'"
                >(公用)</span>
              </div>

              <i 
                v-if="option.id === form_data.tts_model_id"
                class="fas fa-check text-green-500 inline-block ml-2 align-middle transition hover:text-green-700 cursor-pointer"
                @click.stop.prevent="toggleChecked(option)"
              ></i>

              <span 
                v-else 
                class="hidden absolute bottom-[-3px] left-[-4px]" 
                aria-label="$t('common.loading')"
              >
                <svg class="animate-pulse fill-blue-500" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"><defs/><path d="M240 64a128 128 0 10128 0A128 128 0 00240 64zm0 96c-64 0-112 48-112 112s48 112 112 112 112-48 112-112S304 224 240 224zM240 320c-64 0-112 48-112 112s48 112 112 112 112-48 112-112S304 464 240 464z"/></svg>
              </span>

            </el-option>

            <el-option 
              v-for="option in group.filter(item => item.status !== 'SUCCESS')" 
              :key="option.id"
              :label="option.description" // Adjusted property name based on usage in component
              :disabled="true"
              class="flex justify-between px-2 py-1 rounded border border-red-500 transition ease-in-out duration-200 italic opacity-50 select-none whitespace-normal overflow-x-hidden scroll-hidden max-h-40 text-ellipsis truncate">
              
              <div>
                <!-- Same as successful model, just with a different tooltip and styles -->
                
              </div>
            
              <span>(不可用)</span>
            </el-option>

          </el-option-group>

        </el-select>
        

        <el-button 
          v-if="form_data.tts_type === 'TTS'"
          @click="openTTSParamSettingDialog()"
          :disabled="!form_data.tts_model_id || !form_data.tts_model_enable"
          class="ml-8 flex items-center gap-x-2"
        >
          更多设置<i class="las la-settings"></i>
        </el-button>
        
        <el-tooltip effect="dark" placement="bottom-start" content="更多高级参数和配置。"> <!-- Consider adding tooltips for enhanced UX -->
          <!-- Additional context/info if available -->
        </el-tooltip>

      </div>

    </el-flex>

  </el-form-item>
</template>

<script setup lang="ts">
// Import necessary functions / modules from libraries/plugins/services here
  
const props = defineProps<{
  form_data: { [x: string]: any }
}>();

const emit = defineEmits(['refresh']);

function refreshTTSForm() {
  // Implement update functionality here
  emit('refresh');
}

// Add any utility functions such as:
async function openTTSParamSettingDialog() {
  TTSModeParamSettingDialogRef.value.open();
}

const toggleChecked = async (option: any): Promise<void> => {
  return await new Promise(() =>
    setTimeout(resolve => resolve(), Math.floor(Math.random() * (1000)) + 500)
  );
};

// Other lifecycle hooks or watchers will go here too
watch(form_data,'newValue', () => console.log(newValue));
</script>

This revised version focuses on reducing repetition, improving readability, enhancing performance where applicable while maintaining functional integrity, particularly for complex forms involving multiple controls and inter-dependent data fields.

name='tts_autoplay',
field=models.BooleanField(default=False, verbose_name='自动播放'),
),
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The given Python code snippet appears to be part of a migration file for a Django application using version 4.2. Here's a concise review:

Irregularities/Issues:

  1. File Header: The file header is missing the necessary django.core.management.base.BaseCommand import.

    from django.core.management.base.BaseCommand import BaseCommand
  2. Class Name and Methods: Ensure that class attributes like dependencies and operations match those expected by Django migration classes.

  3. Docstring: A docstring explaining the purpose of the module would be helpful for future reference.

  4. Verbose Names: Ensure that all verbose names used with fields (like '自动播放') are correctly localized and available in your locale settings.

Optimization Suggestions:

  • Use Constants for Field Choices if Applicable:
    Instead of hardcoding values in models.BooleanField, consider defining constants:

    AUTOPLAY_CHOICES = [
        (True, _('Automatically play')),
        (False, _('Do not automatically play'))
    ]

    And then use this in the field definition:

    field=models.BooleanField(choices=AUTOPLAY_CHOICES)
  • Consider Adding Default Data During Migration:
    If setting default data during migrations is appropriate, you can do so using the RunPython operation:

    migrations.RunPython(set_default_tts_autoplay),
  • Ensure Proper Imports for Translations:
    Use the correct translator function or path to internationalize the strings.

Example Revised Code:

Here’s an example incorporating these suggestions:

# Generated by Django 4.2.15 on 2025-01-03 14:07

from django.conf import gettext_lazy as _
from django.db import migrations, models


TTS_AUTOPLAY_CHOICES = [
    (True, _('Automatic Play')),
    (False, _('Do Not Automatic Play'))
]

class Migration(migrations.Migration):

    dependencies = [
        ('application', '0021_applicationpublicaccessclient_client_id_and_more'),
    ]

    def set_default_tts_autoplay(apps, schema_editor):
        Application = apps.get_model('application', 'Application')
        Application.objects.all().update(tts_autoplay=True)

    operations = [
        migrations.AddField(
            model_name='application',
            name='tts_autoplay',
            field=models.BooleanField(
                choices=TTS_AUTOPLAY_CHOICES,
                verbose_name=_('Automatic playback'))
        ),
        # Optional: Set a default value for existing entries
        migrations.RunPython(
            set_default_tts_autoplay,
            reverse_code=migrations.RunPython.noop
        )
    ]

This revised code includes proper imports, uses meaningful constant names, provides context in comments where necessary, and suggests adding default data handling through migrations.

@liuruibin liuruibin merged commit 6c44622 into main Jan 3, 2025
4 of 5 checks passed
@liuruibin liuruibin deleted the pr@main@feat_support_autoplay branch January 3, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants